Re: Possible race condition in pg_basebackup

Hello, I'm attaching an updated patch. As suggested, I now attempt to
reserve the WAL earlier in the process, before the call to
do_pg_backup_start(), and I moved the injection point closer towards to
make the test stricter towards the race window. As a side effect, WAL
reservation behaviour change won't apply when the SQL interface
(pg_backup_start()) is invoked, which was highlighted earlier.

Since the bug I intend to fix here has simple workarounds, I don't think
it's necessary to consider backporting the patch to earlier versions,
and the introduction of a new GUC should therefore be acceptable.

The patch is rebased on top of the master as of approximately 2026-09-07
14:50:00 UTC.

I agree that a separate patch for pg_basebackup, to make it request the
slot creation earlier, would be beneficial, and make a separate patch
for that.

Cheers

Nick

On 31/08/2026 13:32, Nick Ivanov wrote:
> 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.
>