Message-ID: <999caa0e-d015-42da-9b79-001087e719b8@enterprisedb.com>
Date: Mon, 31 Aug 2026 13:32:25 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Possible race condition in pg_basebackup
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
 pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
References: <aoh9KdCo12Y07mZO@alvherre.pgsql>
 <5516902D-65A5-4C61-8568-E32F111C89EF@yandex-team.ru>
 <CALP_NYQZHLp4+so=JXJwVonP8Rkz2nx4uwRZOP1DXGtY_gW8eQ@mail.gmail.com>
 <99DF8255-F6B8-4B10-85A2-B6984B3DD16C@yandex-team.ru>
Content-Language: en-US
From: Nick Ivanov <nick.ivanov@enterprisedb.com>
In-Reply-To: <99DF8255-F6B8-4B10-85A2-B6984B3DD16C@yandex-team.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/999caa0e-d015-42da-9b79-001087e719b8%40enterprisedb.com>
Precedence: bulk

Hello Andrey,

Thank you for your comments, they are very helpful.

On 29/08/2026 15:18, Andrey Borodin wrote:
> RegisterBackupStartpoint() is called after the starting checkpoint has
> selected the startpoint.  Another checkpoint can remove WAL between reading
> ControlFile and registering that LSN.  The test stops later, after
> registration, so it does not exercise this window.
> ...
> Avoiding this requires registering a conservative current insert or replay
> position before requesting the starting checkpoint.  If the selected
> startpoint is older, as can happen on a standby, the horizon then has to be
> lowered.  Alternatively, selection and registration need an interlock with
> WAL removal.
I did consider this gap, but thought it wasn't wide enough to be of 
great risk. However, you're right in that, when fixing a race condition 
we should close the gap completely and not resort to a half-measure. I 
will update the test and see where it takes me.
> For a backpatch, I think a new postmaster GUC is a non-starter.  It adds a
> user-visible limit and shared-memory sizing decisions to a minor-version bug
> fix, while concurrent BASE_BACKUP sessions are already bounded by
> max_wal_senders.  Unconditionally registering in do_pg_backup_start() also
> changes SQL-level pg_backup_start().
I didn't like introducing a new GUC myself, but I thought recycling 
max_wal_senders (or max_replication_slots or whatever) would be 
confusing. I believe this race condition can affect `pg_basebackup 
--wal-method=fetch` as well as `--wal-method=stream`, so tying this to 
replication seemed wrong to me. On the other hand, I didn't fully 
consider back-porting the patch, so this needs more thought obviously.
> I suggested server-owned retention upthread, but after reading the patch I
> think your original client-side proposal deserves another look.  Creating
> the requested slot before sending BASE_BACKUP directly fixes the reported
> --create-slot case, is much smaller to backpatch, and also works when a new
> pg_basebackup connects to an older server.  Existing slots with a NULL
> restart_lsn, idle_replication_slot_timeout, and server-fetched WAL can be
> treated as separate server-side problems.  WDYT?
I suspect this won't address the `--wal-method=fetch` situation, which I 
think is subject to the same WAL removal/recycling risk.
> The September Commitfest is open for registration until September 1.  I
> suggest registering the patch now so that further versions and discussion do
> not fall between CommitFests.

Thanks for the advice, will do.

Cheers

Nick