Date: Fri, 12 Mar 2021 16:23:25 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Cc: Andres Freund <andres@anarazel.de>,
	Denis Laxalde <denis.laxalde@dalibo.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Disable bgworkers during servers start in pg_upgrade
Message-ID: <20210312082325.weltivlokfpc3gt2@nol>
References: <20210121152357.s6eflhqyh4g5e6dv@dalibo.com>
 <20210124003611.lkyxhd2mpdmv5xzh@alap3.anarazel.de>
 <20210127102511.tvhu2wmrxoz3tle4@dalibo.com>
 <20210127144132.215fe4bb@firost>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210127144132.215fe4bb@firost>
Archived-At: 
 <https://www.postgresql.org/message-id/20210312082325.weltivlokfpc3gt2%40nol>
Precedence: bulk

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.

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

> What about c) where the bgworker are not loaded by default during binary upgrade
> mode / -b unless they explicitly set a bgw_flags (BGWORKER_BINARY_UPGRADE ?)
> when they are required during pg_upgrade?

As mentioned above +1 for not launching the bgworkers.  Does anyone can think
of a reason why some bgworker would really be needed during pg_upgrade, either
on the source or the target cluster?