Julien Rouhaud a écrit :
On Wed, Jan 27, 2021 at 02:41:32PM +0100, Jehan-Guillaume de Rorthais wrote:
On Wed, 27 Jan 2021 11:25:11 +0100
Denis Laxalde <denis.laxa...@dalibo.com> wrote:
Andres Freund a écrit :
I wonder if we could
a) set default_transaction_read_only to true, and explicitly change it
in the sessions that need that.
According to Denis' tests discussed off-list, it works fine in regard with the
powa bgworker, albeit some complaints in logs. However, I wonder how fragile it
could be as bgworker could easily manipulate either the GUC or "BEGIN READ
WRITE". I realize this is really uncommon practices, but bgworker code from
third parties might be surprising.
Given that having any writes happening at the wrong moment on the old cluster
can end up corrupting the new cluster, and that the corruption might not be
immediately visible we should try to put as many safeguards as possible.
so +1 for the default_transaction_read_only as done in Denis' patch at minimum,
but not only.
AFAICT it should be easy to prevent all background worker from being launched
by adding a check on IsBinaryUpgrade at the beginning of
bgworker_should_start_now(). It won't prevent modules from being loaded, so
this approach should be problematic.
Please find attached another patch implementing this suggestion (as a
complement to the previous patch setting default_transaction_read_only).
b) when in binary upgrade mode / -b, error out on all wal writes in
sessions that don't explicitly set a session-level GUC to allow
writes.
It feels safer because more specific to the subject. But I wonder if the
benefice worth adding some (limited?) complexity and a small/quick conditional
block in a very hot code path for a very limited use case.
I don't think that it would add that much complexity or overhead as there's
already all the infrastructure to prevent WAL writes in certain condition. It
should be enough to add an additional test in XLogInsertAllowed() with some new
variable that is set when starting in binary upgrade mode, and a new function
to disable it that will be emitted by pg_dump / pg_dumpall in binary upgrade
mode.
This part is less clear to me so I'm not sure I'd be able to work on it.
>From b265b6f5b49cfcbc3c6271e980455696a5a3622b Mon Sep 17 00:00:00 2001
From: Denis Laxalde <denis.laxa...@dalibo.com>
Date: Mon, 23 Aug 2021 15:19:41 +0200
Subject: [PATCH] Do not start bgworker processes during binary upgrade
Background workers may produce undesired activities (writes) on the old
cluster during upgrade which may corrupt the new cluster. For a similar
reason that we restrict socket access and set default transactions to
read-only when starting the old cluster in pg_upgrade, we should prevent
bgworkers from starting when using the -b flag.
---
src/backend/postmaster/postmaster.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9c2c98614a..e66c962509 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5862,6 +5862,9 @@ do_start_bgworker(RegisteredBgWorker *rw)
static bool
bgworker_should_start_now(BgWorkerStartTime start_time)
{
+ if (IsBinaryUpgrade)
+ return false;
+
switch (pmState)
{
case PM_NO_CHILDREN:
--
2.30.2
