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
