The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation: not tested
Hi Nick, Andrey, I tested v3 on master (45fc51f, 2026-10-17), built with --enable-cassert --enable-debug --enable-tap-tests --enable-injection-points on Linux (aarch64). v3 no longer applies (cfbot shows "Needs rebase"): 0001 conflicts in src/test/recovery/meson.build, and 057 is now taken by 057_snapshot_commit_race.pl (058-060 exist as well). 0002 applies cleanly. For testing I renamed the test to 061_basebackup_slot_race.pl. - No compiler warnings. - The new test passes with 0001+0002. With 0001 only (the injection point and the test, no fix) it fails in both modes with "requested WAL segment ... has already been removed", as described upthread. - src/bin/pg_basebackup's TAP tests pass with the patch. - Without injection points, the reported scenario: three pg_basebackup -X stream --create-slot -c fast started together, 1 MB WAL segments, wal_keep_size = 0, max_wal_size = 32MB, and a session writing WAL continuously (pg_logical_emit_message, 1 MB each). Over 100 rounds (300 backups), 62 failed without 0002, all with "requested WAL segment ... has already been removed", and none failed with it. All 538 backups that completed pass pg_verifybackup. One behaviour change I noticed: with --create-slot, the permanent slot is now created before BASE_BACKUP, and pg_basebackup doesn't drop it if the backup can't start. Before the patch, a backup the server refused never created the slot. To check: initdb, summarize_wal = on, pg_basebackup a full backup, then summarize_wal = off, then pg_basebackup -X stream --create-slot -S leftover --incremental=<full>/backup_manifest -> could not initiate base backup: ERROR: incremental backups cannot be taken unless WAL summarization is enabled On master no slot is left. With the patch, "leftover" remains with restart_lsn 0/02000028 and keeps holding back WAL until someone drops it. Since the slot is now created first, any other error from BASE_BACKUP should leave it behind the same way (I only tried this one). Temporary slots are not affected, since they go away with the connection. Would it make sense to drop the slot when BASE_BACKUP fails, if this run created it? A failure later in the backup already leaves the slot behind today, so that part isn't new, but a refused start probably shouldn't leave a permanent slot behind. I haven't tried the back branches yet. Regards, Sai Pranav The new status of this patch is: Waiting on Author
