Re: Possible race condition in pg_basebackup

Hi Nick,

Thank you for working on this. I took a look into the patch.

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.

To test it, I would move the injection point from perform_base_backup() to
immediately before RegisterBackupStartpoint(state->startpoint) in
do_pg_backup_start(). While stopped there, remember the selected segment,
generate WAL and run CHECKPOINT. I expect the current patch to lose that
segment.

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.

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 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?

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.

Thank you!

Best regards, Andrey Borodin.