Subject: Re: [PATCH] Disable bgworkers during servers start in pg_upgrade
To: Julien Rouhaud <rjuju123@gmail.com>,
 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org
References: <20210121152357.s6eflhqyh4g5e6dv@dalibo.com>
 <20210124003611.lkyxhd2mpdmv5xzh@alap3.anarazel.de>
 <20210127102511.tvhu2wmrxoz3tle4@dalibo.com> <20210127144132.215fe4bb@firost>
 <20210312082325.weltivlokfpc3gt2@nol>
From: Denis Laxalde <denis.laxalde@dalibo.com>
Message-ID: <e7d8042b-e5a2-6093-e20e-15743c1a2843@dalibo.com>
Date: Tue, 24 Aug 2021 16:40:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20210312082325.weltivlokfpc3gt2@nol>
Content-Type: multipart/mixed;
 boundary="------------C4003EFE459ABA0038C6EAC4"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/e7d8042b-e5a2-6093-e20e-15743c1a2843%40dalibo.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------C4003EFE459ABA0038C6EAC4
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

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.laxalde@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.


--------------C4003EFE459ABA0038C6EAC4
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-Do-not-start-bgworker-processes-during-binary-upgrad.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0001-Do-not-start-bgworker-processes-during-binary-upgrad.pa";
 filename*1="tch"

From b265b6f5b49cfcbc3c6271e980455696a5a3622b Mon Sep 17 00:00:00 2001
From: Denis Laxalde <denis.laxalde@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


--------------C4003EFE459ABA0038C6EAC4--