Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

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(dot)laxalde(at)dalibo(dot)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.