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
