From: Denis Laxalde <denis.laxalde@dalibo.com>
To: Julien Rouhaud <rjuju123@gmail.com>
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Disable bgworkers during servers start in pg_upgrade
Date: Tue, 24 Aug 2021 16:40:02 +0200
Message-ID: <e7d8042b-e5a2-6093-e20e-15743c1a2843@dalibo.com> (raw)
In-Reply-To: <20210312082325.weltivlokfpc3gt2@nol>
References: <20210121152357.s6eflhqyh4g5e6dv@dalibo.com>
<20210124003611.lkyxhd2mpdmv5xzh@alap3.anarazel.de>
<20210127102511.tvhu2wmrxoz3tle4@dalibo.com>
<20210127144132.215fe4bb@firost>
<20210312082325.weltivlokfpc3gt2@nol>
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.
Attachments:0001-Do-not-start-bgworker-processes-during-binary-upgrad.patch (text/x-patch, 1.1K) download
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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: denis.laxalde@dalibo.com, rjuju123@gmail.com, jgdr@dalibo.com, andres@anarazel.de, pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Disable bgworkers during servers start in pg_upgrade
In-Reply-To: <e7d8042b-e5a2-6093-e20e-15743c1a2843@dalibo.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox