From: Nick Ivanov <nick.ivanov@enterprisedb.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Álvaro Herrera <alvherre@kurilemu.de>
Cc: pgsql-hackers mailing list <pgsql-hackers@lists.postgresql.org>
Subject: Re: Possible race condition in pg_basebackup
Date: Mon, 7 Sep 2026 15:19:40 +0100
Message-ID: <0cab2102-4786-416a-ae50-b606a495d01b@enterprisedb.com> (raw)
In-Reply-To: <999caa0e-d015-42da-9b79-001087e719b8@enterprisedb.com>
References: <aoh9KdCo12Y07mZO@alvherre.pgsql>
<5516902D-65A5-4C61-8568-E32F111C89EF@yandex-team.ru>
<CALP_NYQZHLp4+so=JXJwVonP8Rkz2nx4uwRZOP1DXGtY_gW8eQ@mail.gmail.com>
<99DF8255-F6B8-4B10-85A2-B6984B3DD16C@yandex-team.ru>
<999caa0e-d015-42da-9b79-001087e719b8@enterprisedb.com>
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.
>
From 8d6a26b5d271bab4627d3be28ddaf28536cb7a62 Mon Sep 17 00:00:00 2001
From: Nick Ivanov <nick.ivanov@enterprisedb.com>
Date: Sun, 6 Sep 2026 13:31:12 +0100
Subject: [PATCH v2] Fix WAL recycle race in pg_basebackup
Introduce a new shared memory structure to keep the WAL start point for each backup in progress and the corresponding accessor functions to inform the checkpointer of WAL segments we want to keep while the backups are running. Reserve the WAL early in the process, before invoking do_pg_backup_start(), to ensure it happens before the checkpoint.
---
src/backend/access/transam/xlog.c | 40 +++++
src/backend/access/transam/xlogbackup.c | 148 ++++++++++++++++++
src/backend/backup/basebackup.c | 9 ++
.../utils/activity/wait_event_names.txt | 1 +
src/backend/utils/misc/guc_parameters.dat | 8 +
src/backend/utils/misc/postgresql.conf.sample | 2 +
src/include/access/xlogbackup.h | 40 +++++
src/include/storage/lwlocklist.h | 1 +
src/include/storage/subsystemlist.h | 1 +
src/test/recovery/meson.build | 1 +
.../recovery/t/057_basebackup_slot_race.pl | 135 ++++++++++++++++
11 files changed, 386 insertions(+)
create mode 100644 src/test/recovery/t/057_basebackup_slot_race.pl
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 3203f2fd4ee..3c878fd2b70 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -58,6 +58,7 @@
#include "access/xact.h"
#include "access/xlog_internal.h"
#include "access/xlogarchive.h"
+#include "access/xlogbackup.h"
#include "access/xloginsert.h"
#include "access/xlogreader.h"
#include "access/xlogrecovery.h"
@@ -8537,6 +8538,7 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
XLogSegNo currSegNo;
XLogSegNo segno;
XLogRecPtr keep;
+ XLogRecPtr keep_for_backups;
XLByteToSeg(recptr, currSegNo, wal_segment_size);
segno = currSegNo;
@@ -8544,9 +8546,22 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
/* Calculate how many segments are kept by slots. */
keep = XLogGetReplicationSlotMinimumLSN();
if (XLogRecPtrIsValid(keep) && keep < recptr)
+ XLByteToSeg(keep, segno, wal_segment_size);++ /*+ * Check if we need to keep more segments for in-progress backups.+ * This will also be subject to max_slot_wal_keep_size_mb, if set.+ */+ keep_for_backups = GetOldestBackupStartLSN();+ if (XLogRecPtrIsValid(keep_for_backups) &&+ (!XLogRecPtrIsValid(keep) || keep_for_backups < keep))
{
+ keep = keep_for_backups;
XLByteToSeg(keep, segno, wal_segment_size);
+ }+ if (segno < currSegNo)+ {
/*
* Account for max_slot_wal_keep_size to avoid keeping more than
* configured. However, don't do that during a binary upgrade: if
@@ -9695,6 +9710,25 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
WALInsertLockRelease();
} while (!gotUniqueStartpoint);
+ /*+ * The startpoint has been selected, but the client does not know it+ * yet and therefore cannot have created the requested slot.+ */+ INJECTION_POINT("basebackup-before-send-startpoint", NULL);++ /*+ * If we are invoked by basebackup, it may have already reserved a WAL+ * start point. If that is newer than what's selected here, update+ * the reservation.+ */+ if (MyBackupInProgress != NULL &&+ XLogRecPtrIsValid(MyBackupInProgress->startpoint) &&+ state->startpoint < MyBackupInProgress->startpoint)+ {+ UnregisterBackupStartpoint();+ RegisterBackupStartpoint(state->startpoint);+ }+
/*
* Construct tablespace_map file.
*/
@@ -9899,6 +9933,9 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
WALInsertLockRelease();
+ /* Unregister from the shared control structure */+ UnregisterBackupStartpoint();+
/*
* If we are taking an online backup from the standby, we confirm that the
* standby has not been promoted during the backup.
@@ -10134,6 +10171,9 @@ do_pg_abort_backup(int code, Datum arg)
sessionBackupState = SESSION_BACKUP_NONE;
WALInsertLockRelease();
+ /* Unregister from the shared control structure */+ UnregisterBackupStartpoint();+
if (!during_backup_start)
ereport(WARNING,
errmsg("aborting backup due to backend exiting before pg_backup_stop was called"));
diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.cindex cf5cc8ead96..1088eb2d949 100644--- a/src/backend/access/transam/xlogbackup.c+++ b/src/backend/access/transam/xlogbackup.c@@ -16,6 +16,154 @@
#include "access/xlog.h"
#include "access/xlog_internal.h"
#include "access/xlogbackup.h"
+#include "storage/lwlock.h"+#include "storage/shmem.h"++/* Control array for in-progress backups */+BackupCtlData *BackupCtl = NULL;++static void BackupCtlShmemRequest(void *arg);+static void BackupCtlShmemInit(void *arg);++const ShmemCallbacks BackupCtlShmemCallbacks = {+ .request_fn = BackupCtlShmemRequest,+ .init_fn = BackupCtlShmemInit,+};++/* This backend's backup control structure in the shared memory array */+BackupInProgress *MyBackupInProgress = NULL;++/* GUC */+int max_concurrent_backups = 10; /* the maximum number of concurrent backups */++/*+ * Register shared memory space for the backup control structure+ */+static void BackupCtlShmemRequest(void *arg)+{+ Size size;++ /* max_concurrent_backups is at least 1 */+ Assert(max_concurrent_backups > 0);++ size = offsetof(BackupCtlData, backups);+ size = add_size(size, mul_size(max_concurrent_backups, sizeof(BackupInProgress)));+ ShmemRequestStruct(.name = "Backup Ctl",+ .size = size,+ .ptr = (void **)&BackupCtl);+}++/*+ * Initialize shared memory for the backup control structure.+ *+ * No cleanup is needed on shmem_exit.+ */+static void BackupCtlShmemInit(void *arg)+{+ int i;++ for (i = 0; i < max_concurrent_backups; i++)+ {+ BackupCtl->backups[i].startpoint = InvalidXLogRecPtr;+ }+ BackupCtl->oldestStartpoint = InvalidXLogRecPtr;+}++/*+ * Register this backup's startpoint.+ *+ * We update the oldest startpoint across all in-progress backups here.+ *+ */++void+RegisterBackupStartpoint(XLogRecPtr startpoint)+{+ int i;++ Assert(BackupCtl != NULL);+ Assert(MyBackupInProgress == NULL);++ LWLockAcquire(BackupControlLock, LW_EXCLUSIVE);+ for (i = 0; i < max_concurrent_backups; i++)+ {+ /* Find the first unused entry */+ if (BackupCtl->backups[i].startpoint == InvalidXLogRecPtr)+ {+ BackupCtl->backups[i].startpoint = startpoint;+ MyBackupInProgress = &BackupCtl->backups[i];+ /* Update the oldest startpoint if necessary */+ if (!XLogRecPtrIsValid(BackupCtl->oldestStartpoint) ||+ startpoint < BackupCtl->oldestStartpoint)+ BackupCtl->oldestStartpoint = startpoint;+ break;+ }+ }+ LWLockRelease(BackupControlLock);++ /* If the array is full, bail out */+ if (i == max_concurrent_backups)+ ereport(ERROR,+ (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),+ errmsg("maximum number of concurrent backups reached"),+ errhint("Wait for another backup to finish, or increase max_concurrent_backups.")));+}++/*+ * Unregister this backup.+ *+ * We also recalculate the oldest startpoint across all remaining+ * in-progress backups.+ *+ */++void+UnregisterBackupStartpoint(void)+{+ XLogRecPtr candidate_startpoint;++ if (MyBackupInProgress == NULL)+ return;++ Assert(BackupCtl != NULL);++ LWLockAcquire(BackupControlLock, LW_EXCLUSIVE);++ MyBackupInProgress->startpoint = InvalidXLogRecPtr;++ /* Invalidate the oldest startpoint */+ BackupCtl->oldestStartpoint = InvalidXLogRecPtr;+ /* Scan the array to find the new oldest startpoint */+ for (int i = 0; i < max_concurrent_backups; i++)+ {+ candidate_startpoint = BackupCtl->backups[i].startpoint;+ if (XLogRecPtrIsValid(candidate_startpoint) &&+ (!XLogRecPtrIsValid(BackupCtl->oldestStartpoint) ||+ candidate_startpoint < BackupCtl->oldestStartpoint))+ BackupCtl->oldestStartpoint = candidate_startpoint;+ }++ LWLockRelease(BackupControlLock);++ MyBackupInProgress = NULL;+}++/*+ * Return the precomputed minimum startpoint across all in-progress backups+ * to use when determining what WAL segments to keep.+ */++XLogRecPtr+GetOldestBackupStartLSN(void)+{+ XLogRecPtr retval;++ LWLockAcquire(BackupControlLock, LW_SHARED);+ retval = BackupCtl->oldestStartpoint;+ LWLockRelease(BackupControlLock);++ return retval;+}
/*
* Build contents for backup_label or backup history file.
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.cindex e3c04ecd810..8bdbb355b7f 100644--- a/src/backend/backup/basebackup.c+++ b/src/backend/backup/basebackup.c@@ -18,6 +18,7 @@
#include "access/xlog_internal.h"
#include "access/xlogbackup.h"
+#include "access/xlogrecovery.h"
#include "backup/backup_manifest.h"
#include "backup/basebackup.h"
#include "backup/basebackup_incremental.h"
@@ -244,6 +245,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
{
bbsink_state state;
XLogRecPtr endptr;
+ XLogRecPtr reserve_ptr;
TimeLineID endtli;
backup_manifest_info manifest;
BackupState *backup_state;
@@ -264,6 +266,13 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
backup_started_in_recovery = RecoveryInProgress();
+ /* Reserve the XLog before starting the backup */+ if (backup_started_in_recovery)+ reserve_ptr = GetXLogReplayRecPtr(NULL);+ else+ reserve_ptr = GetXLogInsertRecPtr();+ RegisterBackupStartpoint(reserve_ptr);+
InitializeBackupManifest(&manifest, opt->manifest,
opt->manifest_checksum_type);
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txtindex 0a70cffa081..bb01fcf7f1a 100644--- a/src/backend/utils/activity/wait_event_names.txt+++ b/src/backend/utils/activity/wait_event_names.txt@@ -371,6 +371,7 @@ WaitLSN "Waiting to read or update shared Wait-for-LSN state."
LogicalDecodingControl "Waiting to read or update logical decoding status information."
DataChecksumsWorker "Waiting for data checksums worker."
AioWorkerControl "Waiting to update AIO worker information."
+BackupControl "Waiting to update basebackup state."
#
# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.datindex 3c5e16ad1e7..20dbe3946ec 100644--- a/src/backend/utils/misc/guc_parameters.dat+++ b/src/backend/utils/misc/guc_parameters.dat@@ -1983,6 +1983,14 @@
max => 'MAX_BACKENDS',
},
+{ name => 'max_concurrent_backups', type => 'int', context => 'PGC_POSTMASTER', group => 'REPLICATION_SENDING',+ short_desc => 'Sets the maximum number of simultaneously running basebackups.',+ variable => 'max_concurrent_backups',+ boot_val => '10',+ min => '1',+ max => 'MAX_BACKENDS',+},+
{ name => 'max_connections', type => 'int', context => 'PGC_POSTMASTER', group => 'CONN_AUTH_SETTINGS',
short_desc => 'Sets the maximum number of concurrent connections.',
variable => 'MaxConnections',
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sampleindex e759f06b50f..186a2774ef4 100644--- a/src/backend/utils/misc/postgresql.conf.sample+++ b/src/backend/utils/misc/postgresql.conf.sample@@ -275,6 +275,8 @@
#commit_delay = 0 # range 0-100000, in microseconds
#commit_siblings = 5 # range 0-1000
+#max_concurrent_backups = 10 # range 1-MAX_BACKENDS +
# - Checkpoints -
#checkpoint_timeout = 5min # range 30s-1d
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.hindex 2cc2f85d9f0..5d997cbfcd7 100644--- a/src/include/access/xlogbackup.h+++ b/src/include/access/xlogbackup.h@@ -37,6 +37,46 @@ typedef struct BackupState
pg_time_t stoptime; /* backup stop time */
} BackupState;
+/*+ * Shared memory state of a backup in progress. Here we keep track of its+ * start LSN to ensure checkpoints don't recycle or remove the corresponding+ * WAL segments until we're done.+ *+ * Using a struct instead of a bare XLogRecPtr to allow future extensions.+ */+typedef struct BackupInProgress {+ /*+ * Each backup triggers its own checkpoint, so their startpoints are+ * guaranteed to be unique, and we can use InvalidXLogRecPtr to indicate+ * an available entry+ */+ XLogRecPtr startpoint;+} BackupInProgress;++/* Shared memory structure for all in-progress backups+ *+ * It is protected by the LWLock BackupControlLock; exclusive+ * for writers, shared for readers.+ */+typedef struct BackupCtlData {+ /* Minimum startpoint of all in-progress backups */+ XLogRecPtr oldestStartpoint;+ BackupInProgress backups[FLEXIBLE_ARRAY_MEMBER];+} BackupCtlData;++/*+ * Pointer to shared memory+ */+extern PGDLLIMPORT BackupCtlData *BackupCtl;+extern PGDLLIMPORT BackupInProgress *MyBackupInProgress;++/* GUCs */+extern PGDLLIMPORT int max_concurrent_backups;++extern void RegisterBackupStartpoint(XLogRecPtr startpoint);+extern void UnregisterBackupStartpoint(void);+extern XLogRecPtr GetOldestBackupStartLSN(void);+
extern char *build_backup_content(BackupState *state,
bool ishistoryfile);
diff --git a/src/include/storage/lwlocklist.h b/src/include/storage/lwlocklist.hindex d7eb648bd27..80dfc5775a1 100644--- a/src/include/storage/lwlocklist.h+++ b/src/include/storage/lwlocklist.h@@ -89,6 +89,7 @@ PG_LWLOCK(54, WaitLSN)
PG_LWLOCK(55, LogicalDecodingControl)
PG_LWLOCK(56, DataChecksumsWorker)
PG_LWLOCK(57, AioWorkerControl)
+PG_LWLOCK(58, BackupControl)
/*
* There also exist several built-in LWLock tranches. As with the predefined
diff --git a/src/include/storage/subsystemlist.h b/src/include/storage/subsystemlist.hindex 9ad619080be..d8d1c469226 100644--- a/src/include/storage/subsystemlist.h+++ b/src/include/storage/subsystemlist.h@@ -85,6 +85,7 @@ PG_SHMEM_SUBSYSTEM(InjectionPointShmemCallbacks)
PG_SHMEM_SUBSYSTEM(WaitLSNShmemCallbacks)
PG_SHMEM_SUBSYSTEM(LogicalDecodingCtlShmemCallbacks)
PG_SHMEM_SUBSYSTEM(DataChecksumsShmemCallbacks)
+PG_SHMEM_SUBSYSTEM(BackupCtlShmemCallbacks)
/* AIO subsystem. This delegates to the method-specific callbacks */
PG_SHMEM_SUBSYSTEM(AioShmemCallbacks)
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.buildindex 72113c5ac6e..db045f9cc1f 100644--- a/src/test/recovery/meson.build+++ b/src/test/recovery/meson.build@@ -65,6 +65,7 @@ tests += {
't/054_unlogged_sequence_promotion.pl',
't/055_cascade_reconnect.pl',
't/056_standby_snapshot_export.pl',
+ 't/057_basebackup_slot_race.pl',
],
},
}
diff --git a/src/test/recovery/t/057_basebackup_slot_race.pl b/src/test/recovery/t/057_basebackup_slot_race.plnew file mode 100644index 00000000000..2e83f825cae--- /dev/null+++ b/src/test/recovery/t/057_basebackup_slot_race.pl@@ -0,0 +1,135 @@+# Copyright (c) 2026, PostgreSQL Global Development Group++# Verify that a base backup's startpoint survives WAL recycling triggered by+# a concurrent checkpoint, even before pg_basebackup has created its own+# replication slot.+#+# The injection point stops BASE_BACKUP after choosing its startpoint but+# before sending it to the client. The test recycles WAL up to and including+# the startpoint's segment while BASE_BACKUP is paused there, then lets+# pg_basebackup create its slot and start streaming. The test passes when+# the startpoint's WAL segment is still on disk and pg_basebackup succeeds,+# proving that do_pg_backup_start() protects the segment before the slot+# exists.++use strict;+use warnings FATAL => 'all';+use File::Path qw(rmtree);+use PostgreSQL::Test::Cluster;+use PostgreSQL::Test::Utils;+use Test::More;++if ($ENV{enable_injection_points} ne 'yes')+{+ plan skip_all => 'Injection points not supported by this build';+}++# Small WAL segments make recycling cheap.+my $node = PostgreSQL::Test::Cluster->new('primary');+$node->init(allows_streaming => 1, extra => [ '--wal-segsize', '1' ]);+$node->append_conf(+ 'postgresql.conf', q[+wal_keep_size = 0+min_wal_size = 2MB+max_wal_size = 4MB+checkpoint_timeout = 1h+]);+$node->start;++# injection_points may not be installed under installcheck.+if (!$node->check_extension('injection_points'))+{+ plan skip_all => 'Extension injection_points not installed';+}+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');++# Stop BASE_BACKUP before it sends the selected startpoint to the client.+$node->safe_psql('postgres',+ "SELECT injection_points_attach('basebackup-before-send-startpoint', 'wait');"+);++my $backupdir = $node->backup_dir . '/basebackup_race';+my ($bb_stdout, $bb_stderr) = ('', '');+my $bb_timeout =+ IPC::Run::timeout(3 * $PostgreSQL::Test::Utils::timeout_default);+my $bb = IPC::Run::start(+ [+ 'pg_basebackup',+ '--pgdata' => $backupdir,+ '--wal-method' => 'stream',+ '--slot' => 'basebackup_race',+ '--create-slot',+ '--checkpoint' => 'fast',+ '--no-sync',+ '-d' => $node->connstr('postgres')+ ],+ '>' => \$bb_stdout,+ '2>' => \$bb_stderr,+ $bb_timeout);++$node->wait_for_event('walsender', 'basebackup-before-send-startpoint');++# The client cannot have created its slot yet, since it hasn't received the+# startpoint; the backup's startpoint is instead protected by the shared+# in-progress-backup registry at this point.+is( $node->safe_psql(+ 'postgres', 'SELECT count(*) FROM pg_replication_slots;'),+ '0',+ 'no replication slot exists yet while startpoint is held only by the '+ . 'backup registry');++# do_pg_backup_start() used the current checkpoint's REDO pointer.+my $startpoint_wal = $node->safe_psql('postgres',+ 'SELECT pg_walfile_name(redo_lsn) FROM pg_control_checkpoint();');+note "backup startpoint is in WAL segment $startpoint_wal";++is( $node->safe_psql(+ 'postgres',+ "SELECT count(*) FROM pg_ls_waldir() WHERE name = '$startpoint_wal';"+ ),+ '1',+ 'WAL segment containing the backup startpoint exists while waiting');++# Force enough WAL activity and a checkpoint to make the server want to+# recycle the startpoint's segment. The backup registry should keep it+# around anyway, even though no replication slot protects it yet.+$node->advance_wal(10);+$node->safe_psql('postgres', 'CHECKPOINT;');+is( $node->safe_psql(+ 'postgres',+ "SELECT count(*) FROM pg_ls_waldir() WHERE name = '$startpoint_wal';"+ ),+ '1',+ 'WAL segment containing the backup startpoint survives a concurrent '+ . 'checkpoint'+);++# Let pg_basebackup create the slot and request WAL starting at the+# (still-present) startpoint.+$node->safe_psql('postgres',+ "SELECT injection_points_wakeup('basebackup-before-send-startpoint');");+$node->safe_psql('postgres',+ "SELECT injection_points_detach('basebackup-before-send-startpoint');");++$bb->finish;+note "pg_basebackup stderr:\n$bb_stderr";++is($bb->result(0), 0, 'pg_basebackup succeeded despite concurrent WAL recycling')+ or diag "pg_basebackup stdout: $bb_stdout\npg_basebackup stderr: $bb_stderr";++# The slot requested via --create-slot should now exist.+is( $node->safe_psql(+ 'postgres',+ "SELECT count(*) FROM pg_replication_slots WHERE slot_name = "+ . "'basebackup_race';"),+ '1',+ 'replication slot was created once pg_basebackup received the startpoint'+);++rmtree($backupdir);+$node->safe_psql('postgres',+ "SELECT pg_drop_replication_slot(slot_name) FROM pg_replication_slots "+ . "WHERE slot_name = 'basebackup_race';"+);++done_testing();
--
2.50.1 (Apple Git-155)
Attachments:
[text/plain] v2-0001-Fix-WAL-recycle-race-in-pg_basebackup.patch (19.3K, ../0cab2102-4786-416a-ae50-b606a495d01b@enterprisedb.com/2-v2-0001-Fix-WAL-recycle-race-in-pg_basebackup.patch)
download | inline diff:
From 8d6a26b5d271bab4627d3be28ddaf28536cb7a62 Mon Sep 17 00:00:00 2001
From: Nick Ivanov <nick.ivanov@enterprisedb.com>
Date: Sun, 6 Sep 2026 13:31:12 +0100
Subject: [PATCH v2] Fix WAL recycle race in pg_basebackup
Introduce a new shared memory structure to keep the WAL start point for each backup in progress and the corresponding accessor functions to inform the checkpointer of WAL segments we want to keep while the backups are running. Reserve the WAL early in the process, before invoking do_pg_backup_start(), to ensure it happens before the checkpoint.
---
src/backend/access/transam/xlog.c | 40 +++++
src/backend/access/transam/xlogbackup.c | 148 ++++++++++++++++++
src/backend/backup/basebackup.c | 9 ++
.../utils/activity/wait_event_names.txt | 1 +
src/backend/utils/misc/guc_parameters.dat | 8 +
src/backend/utils/misc/postgresql.conf.sample | 2 +
src/include/access/xlogbackup.h | 40 +++++
src/include/storage/lwlocklist.h | 1 +
src/include/storage/subsystemlist.h | 1 +
src/test/recovery/meson.build | 1 +
.../recovery/t/057_basebackup_slot_race.pl | 135 ++++++++++++++++
11 files changed, 386 insertions(+)
create mode 100644 src/test/recovery/t/057_basebackup_slot_race.pl
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 3203f2fd4ee..3c878fd2b70 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -58,6 +58,7 @@
#include "access/xact.h"
#include "access/xlog_internal.h"
#include "access/xlogarchive.h"
+#include "access/xlogbackup.h"
#include "access/xloginsert.h"
#include "access/xlogreader.h"
#include "access/xlogrecovery.h"
@@ -8537,6 +8538,7 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
XLogSegNo currSegNo;
XLogSegNo segno;
XLogRecPtr keep;
+ XLogRecPtr keep_for_backups;
XLByteToSeg(recptr, currSegNo, wal_segment_size);
segno = currSegNo;
@@ -8544,9 +8546,22 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
/* Calculate how many segments are kept by slots. */
keep = XLogGetReplicationSlotMinimumLSN();
if (XLogRecPtrIsValid(keep) && keep < recptr)
+ XLByteToSeg(keep, segno, wal_segment_size);++ /*+ * Check if we need to keep more segments for in-progress backups.+ * This will also be subject to max_slot_wal_keep_size_mb, if set.+ */+ keep_for_backups = GetOldestBackupStartLSN();+ if (XLogRecPtrIsValid(keep_for_backups) &&+ (!XLogRecPtrIsValid(keep) || keep_for_backups < keep))
{
+ keep = keep_for_backups;
XLByteToSeg(keep, segno, wal_segment_size);
+ }+ if (segno < currSegNo)+ {
/*
* Account for max_slot_wal_keep_size to avoid keeping more than
* configured. However, don't do that during a binary upgrade: if
@@ -9695,6 +9710,25 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
WALInsertLockRelease();
} while (!gotUniqueStartpoint);
+ /*+ * The startpoint has been selected, but the client does not know it+ * yet and therefore cannot have created the requested slot.+ */+ INJECTION_POINT("basebackup-before-send-startpoint", NULL);++ /*+ * If we are invoked by basebackup, it may have already reserved a WAL+ * start point. If that is newer than what's selected here, update+ * the reservation.+ */+ if (MyBackupInProgress != NULL &&+ XLogRecPtrIsValid(MyBackupInProgress->startpoint) &&+ state->startpoint < MyBackupInProgress->startpoint)+ {+ UnregisterBackupStartpoint();+ RegisterBackupStartpoint(state->startpoint);+ }+
/*
* Construct tablespace_map file.
*/
@@ -9899,6 +9933,9 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
WALInsertLockRelease();
+ /* Unregister from the shared control structure */+ UnregisterBackupStartpoint();+
/*
* If we are taking an online backup from the standby, we confirm that the
* standby has not been promoted during the backup.
@@ -10134,6 +10171,9 @@ do_pg_abort_backup(int code, Datum arg)
sessionBackupState = SESSION_BACKUP_NONE;
WALInsertLockRelease();
+ /* Unregister from the shared control structure */+ UnregisterBackupStartpoint();+
if (!during_backup_start)
ereport(WARNING,
errmsg("aborting backup due to backend exiting before pg_backup_stop was called"));
diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.cindex cf5cc8ead96..1088eb2d949 100644--- a/src/backend/access/transam/xlogbackup.c+++ b/src/backend/access/transam/xlogbackup.c@@ -16,6 +16,154 @@
#include "access/xlog.h"
#include "access/xlog_internal.h"
#include "access/xlogbackup.h"
+#include "storage/lwlock.h"+#include "storage/shmem.h"++/* Control array for in-progress backups */+BackupCtlData *BackupCtl = NULL;++static void BackupCtlShmemRequest(void *arg);+static void BackupCtlShmemInit(void *arg);++const ShmemCallbacks BackupCtlShmemCallbacks = {+ .request_fn = BackupCtlShmemRequest,+ .init_fn = BackupCtlShmemInit,+};++/* This backend's backup control structure in the shared memory array */+BackupInProgress *MyBackupInProgress = NULL;++/* GUC */+int max_concurrent_backups = 10; /* the maximum number of concurrent backups */++/*+ * Register shared memory space for the backup control structure+ */+static void BackupCtlShmemRequest(void *arg)+{+ Size size;++ /* max_concurrent_backups is at least 1 */+ Assert(max_concurrent_backups > 0);++ size = offsetof(BackupCtlData, backups);+ size = add_size(size, mul_size(max_concurrent_backups, sizeof(BackupInProgress)));+ ShmemRequestStruct(.name = "Backup Ctl",+ .size = size,+ .ptr = (void **)&BackupCtl);+}++/*+ * Initialize shared memory for the backup control structure.+ *+ * No cleanup is needed on shmem_exit.+ */+static void BackupCtlShmemInit(void *arg)+{+ int i;++ for (i = 0; i < max_concurrent_backups; i++)+ {+ BackupCtl->backups[i].startpoint = InvalidXLogRecPtr;+ }+ BackupCtl->oldestStartpoint = InvalidXLogRecPtr;+}++/*+ * Register this backup's startpoint.+ *+ * We update the oldest startpoint across all in-progress backups here.+ *+ */++void+RegisterBackupStartpoint(XLogRecPtr startpoint)+{+ int i;++ Assert(BackupCtl != NULL);+ Assert(MyBackupInProgress == NULL);++ LWLockAcquire(BackupControlLock, LW_EXCLUSIVE);+ for (i = 0; i < max_concurrent_backups; i++)+ {+ /* Find the first unused entry */+ if (BackupCtl->backups[i].startpoint == InvalidXLogRecPtr)+ {+ BackupCtl->backups[i].startpoint = startpoint;+ MyBackupInProgress = &BackupCtl->backups[i];+ /* Update the oldest startpoint if necessary */+ if (!XLogRecPtrIsValid(BackupCtl->oldestStartpoint) ||+ startpoint < BackupCtl->oldestStartpoint)+ BackupCtl->oldestStartpoint = startpoint;+ break;+ }+ }+ LWLockRelease(BackupControlLock);++ /* If the array is full, bail out */+ if (i == max_concurrent_backups)+ ereport(ERROR,+ (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),+ errmsg("maximum number of concurrent backups reached"),+ errhint("Wait for another backup to finish, or increase max_concurrent_backups.")));+}++/*+ * Unregister this backup.+ *+ * We also recalculate the oldest startpoint across all remaining+ * in-progress backups.+ *+ */++void+UnregisterBackupStartpoint(void)+{+ XLogRecPtr candidate_startpoint;++ if (MyBackupInProgress == NULL)+ return;++ Assert(BackupCtl != NULL);++ LWLockAcquire(BackupControlLock, LW_EXCLUSIVE);++ MyBackupInProgress->startpoint = InvalidXLogRecPtr;++ /* Invalidate the oldest startpoint */+ BackupCtl->oldestStartpoint = InvalidXLogRecPtr;+ /* Scan the array to find the new oldest startpoint */+ for (int i = 0; i < max_concurrent_backups; i++)+ {+ candidate_startpoint = BackupCtl->backups[i].startpoint;+ if (XLogRecPtrIsValid(candidate_startpoint) &&+ (!XLogRecPtrIsValid(BackupCtl->oldestStartpoint) ||+ candidate_startpoint < BackupCtl->oldestStartpoint))+ BackupCtl->oldestStartpoint = candidate_startpoint;+ }++ LWLockRelease(BackupControlLock);++ MyBackupInProgress = NULL;+}++/*+ * Return the precomputed minimum startpoint across all in-progress backups+ * to use when determining what WAL segments to keep.+ */++XLogRecPtr+GetOldestBackupStartLSN(void)+{+ XLogRecPtr retval;++ LWLockAcquire(BackupControlLock, LW_SHARED);+ retval = BackupCtl->oldestStartpoint;+ LWLockRelease(BackupControlLock);++ return retval;+}
/*
* Build contents for backup_label or backup history file.
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.cindex e3c04ecd810..8bdbb355b7f 100644--- a/src/backend/backup/basebackup.c+++ b/src/backend/backup/basebackup.c@@ -18,6 +18,7 @@
#include "access/xlog_internal.h"
#include "access/xlogbackup.h"
+#include "access/xlogrecovery.h"
#include "backup/backup_manifest.h"
#include "backup/basebackup.h"
#include "backup/basebackup_incremental.h"
@@ -244,6 +245,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
{
bbsink_state state;
XLogRecPtr endptr;
+ XLogRecPtr reserve_ptr;
TimeLineID endtli;
backup_manifest_info manifest;
BackupState *backup_state;
@@ -264,6 +266,13 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
backup_started_in_recovery = RecoveryInProgress();
+ /* Reserve the XLog before starting the backup */+ if (backup_started_in_recovery)+ reserve_ptr = GetXLogReplayRecPtr(NULL);+ else+ reserve_ptr = GetXLogInsertRecPtr();+ RegisterBackupStartpoint(reserve_ptr);+
InitializeBackupManifest(&manifest, opt->manifest,
opt->manifest_checksum_type);
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txtindex 0a70cffa081..bb01fcf7f1a 100644--- a/src/backend/utils/activity/wait_event_names.txt+++ b/src/backend/utils/activity/wait_event_names.txt@@ -371,6 +371,7 @@ WaitLSN "Waiting to read or update shared Wait-for-LSN state."
LogicalDecodingControl "Waiting to read or update logical decoding status information."
DataChecksumsWorker "Waiting for data checksums worker."
AioWorkerControl "Waiting to update AIO worker information."
+BackupControl "Waiting to update basebackup state."
#
# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.datindex 3c5e16ad1e7..20dbe3946ec 100644--- a/src/backend/utils/misc/guc_parameters.dat+++ b/src/backend/utils/misc/guc_parameters.dat@@ -1983,6 +1983,14 @@
max => 'MAX_BACKENDS',
},
+{ name => 'max_concurrent_backups', type => 'int', context => 'PGC_POSTMASTER', group => 'REPLICATION_SENDING',+ short_desc => 'Sets the maximum number of simultaneously running basebackups.',+ variable => 'max_concurrent_backups',+ boot_val => '10',+ min => '1',+ max => 'MAX_BACKENDS',+},+
{ name => 'max_connections', type => 'int', context => 'PGC_POSTMASTER', group => 'CONN_AUTH_SETTINGS',
short_desc => 'Sets the maximum number of concurrent connections.',
variable => 'MaxConnections',
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sampleindex e759f06b50f..186a2774ef4 100644--- a/src/backend/utils/misc/postgresql.conf.sample+++ b/src/backend/utils/misc/postgresql.conf.sample@@ -275,6 +275,8 @@
#commit_delay = 0 # range 0-100000, in microseconds
#commit_siblings = 5 # range 0-1000
+#max_concurrent_backups = 10 # range 1-MAX_BACKENDS +
# - Checkpoints -
#checkpoint_timeout = 5min # range 30s-1d
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.hindex 2cc2f85d9f0..5d997cbfcd7 100644--- a/src/include/access/xlogbackup.h+++ b/src/include/access/xlogbackup.h@@ -37,6 +37,46 @@ typedef struct BackupState
pg_time_t stoptime; /* backup stop time */
} BackupState;
+/*+ * Shared memory state of a backup in progress. Here we keep track of its+ * start LSN to ensure checkpoints don't recycle or remove the corresponding+ * WAL segments until we're done.+ *+ * Using a struct instead of a bare XLogRecPtr to allow future extensions.+ */+typedef struct BackupInProgress {+ /*+ * Each backup triggers its own checkpoint, so their startpoints are+ * guaranteed to be unique, and we can use InvalidXLogRecPtr to indicate+ * an available entry+ */+ XLogRecPtr startpoint;+} BackupInProgress;++/* Shared memory structure for all in-progress backups+ *+ * It is protected by the LWLock BackupControlLock; exclusive+ * for writers, shared for readers.+ */+typedef struct BackupCtlData {+ /* Minimum startpoint of all in-progress backups */+ XLogRecPtr oldestStartpoint;+ BackupInProgress backups[FLEXIBLE_ARRAY_MEMBER];+} BackupCtlData;++/*+ * Pointer to shared memory+ */+extern PGDLLIMPORT BackupCtlData *BackupCtl;+extern PGDLLIMPORT BackupInProgress *MyBackupInProgress;++/* GUCs */+extern PGDLLIMPORT int max_concurrent_backups;++extern void RegisterBackupStartpoint(XLogRecPtr startpoint);+extern void UnregisterBackupStartpoint(void);+extern XLogRecPtr GetOldestBackupStartLSN(void);+
extern char *build_backup_content(BackupState *state,
bool ishistoryfile);
diff --git a/src/include/storage/lwlocklist.h b/src/include/storage/lwlocklist.hindex d7eb648bd27..80dfc5775a1 100644--- a/src/include/storage/lwlocklist.h+++ b/src/include/storage/lwlocklist.h@@ -89,6 +89,7 @@ PG_LWLOCK(54, WaitLSN)
PG_LWLOCK(55, LogicalDecodingControl)
PG_LWLOCK(56, DataChecksumsWorker)
PG_LWLOCK(57, AioWorkerControl)
+PG_LWLOCK(58, BackupControl)
/*
* There also exist several built-in LWLock tranches. As with the predefined
diff --git a/src/include/storage/subsystemlist.h b/src/include/storage/subsystemlist.hindex 9ad619080be..d8d1c469226 100644--- a/src/include/storage/subsystemlist.h+++ b/src/include/storage/subsystemlist.h@@ -85,6 +85,7 @@ PG_SHMEM_SUBSYSTEM(InjectionPointShmemCallbacks)
PG_SHMEM_SUBSYSTEM(WaitLSNShmemCallbacks)
PG_SHMEM_SUBSYSTEM(LogicalDecodingCtlShmemCallbacks)
PG_SHMEM_SUBSYSTEM(DataChecksumsShmemCallbacks)
+PG_SHMEM_SUBSYSTEM(BackupCtlShmemCallbacks)
/* AIO subsystem. This delegates to the method-specific callbacks */
PG_SHMEM_SUBSYSTEM(AioShmemCallbacks)
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.buildindex 72113c5ac6e..db045f9cc1f 100644--- a/src/test/recovery/meson.build+++ b/src/test/recovery/meson.build@@ -65,6 +65,7 @@ tests += {
't/054_unlogged_sequence_promotion.pl',
't/055_cascade_reconnect.pl',
't/056_standby_snapshot_export.pl',
+ 't/057_basebackup_slot_race.pl',
],
},
}
diff --git a/src/test/recovery/t/057_basebackup_slot_race.pl b/src/test/recovery/t/057_basebackup_slot_race.plnew file mode 100644index 00000000000..2e83f825cae--- /dev/null+++ b/src/test/recovery/t/057_basebackup_slot_race.pl@@ -0,0 +1,135 @@+# Copyright (c) 2026, PostgreSQL Global Development Group++# Verify that a base backup's startpoint survives WAL recycling triggered by+# a concurrent checkpoint, even before pg_basebackup has created its own+# replication slot.+#+# The injection point stops BASE_BACKUP after choosing its startpoint but+# before sending it to the client. The test recycles WAL up to and including+# the startpoint's segment while BASE_BACKUP is paused there, then lets+# pg_basebackup create its slot and start streaming. The test passes when+# the startpoint's WAL segment is still on disk and pg_basebackup succeeds,+# proving that do_pg_backup_start() protects the segment before the slot+# exists.++use strict;+use warnings FATAL => 'all';+use File::Path qw(rmtree);+use PostgreSQL::Test::Cluster;+use PostgreSQL::Test::Utils;+use Test::More;++if ($ENV{enable_injection_points} ne 'yes')+{+ plan skip_all => 'Injection points not supported by this build';+}++# Small WAL segments make recycling cheap.+my $node = PostgreSQL::Test::Cluster->new('primary');+$node->init(allows_streaming => 1, extra => [ '--wal-segsize', '1' ]);+$node->append_conf(+ 'postgresql.conf', q[+wal_keep_size = 0+min_wal_size = 2MB+max_wal_size = 4MB+checkpoint_timeout = 1h+]);+$node->start;++# injection_points may not be installed under installcheck.+if (!$node->check_extension('injection_points'))+{+ plan skip_all => 'Extension injection_points not installed';+}+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');++# Stop BASE_BACKUP before it sends the selected startpoint to the client.+$node->safe_psql('postgres',+ "SELECT injection_points_attach('basebackup-before-send-startpoint', 'wait');"+);++my $backupdir = $node->backup_dir . '/basebackup_race';+my ($bb_stdout, $bb_stderr) = ('', '');+my $bb_timeout =+ IPC::Run::timeout(3 * $PostgreSQL::Test::Utils::timeout_default);+my $bb = IPC::Run::start(+ [+ 'pg_basebackup',+ '--pgdata' => $backupdir,+ '--wal-method' => 'stream',+ '--slot' => 'basebackup_race',+ '--create-slot',+ '--checkpoint' => 'fast',+ '--no-sync',+ '-d' => $node->connstr('postgres')+ ],+ '>' => \$bb_stdout,+ '2>' => \$bb_stderr,+ $bb_timeout);++$node->wait_for_event('walsender', 'basebackup-before-send-startpoint');++# The client cannot have created its slot yet, since it hasn't received the+# startpoint; the backup's startpoint is instead protected by the shared+# in-progress-backup registry at this point.+is( $node->safe_psql(+ 'postgres', 'SELECT count(*) FROM pg_replication_slots;'),+ '0',+ 'no replication slot exists yet while startpoint is held only by the '+ . 'backup registry');++# do_pg_backup_start() used the current checkpoint's REDO pointer.+my $startpoint_wal = $node->safe_psql('postgres',+ 'SELECT pg_walfile_name(redo_lsn) FROM pg_control_checkpoint();');+note "backup startpoint is in WAL segment $startpoint_wal";++is( $node->safe_psql(+ 'postgres',+ "SELECT count(*) FROM pg_ls_waldir() WHERE name = '$startpoint_wal';"+ ),+ '1',+ 'WAL segment containing the backup startpoint exists while waiting');++# Force enough WAL activity and a checkpoint to make the server want to+# recycle the startpoint's segment. The backup registry should keep it+# around anyway, even though no replication slot protects it yet.+$node->advance_wal(10);+$node->safe_psql('postgres', 'CHECKPOINT;');+is( $node->safe_psql(+ 'postgres',+ "SELECT count(*) FROM pg_ls_waldir() WHERE name = '$startpoint_wal';"+ ),+ '1',+ 'WAL segment containing the backup startpoint survives a concurrent '+ . 'checkpoint'+);++# Let pg_basebackup create the slot and request WAL starting at the+# (still-present) startpoint.+$node->safe_psql('postgres',+ "SELECT injection_points_wakeup('basebackup-before-send-startpoint');");+$node->safe_psql('postgres',+ "SELECT injection_points_detach('basebackup-before-send-startpoint');");++$bb->finish;+note "pg_basebackup stderr:\n$bb_stderr";++is($bb->result(0), 0, 'pg_basebackup succeeded despite concurrent WAL recycling')+ or diag "pg_basebackup stdout: $bb_stdout\npg_basebackup stderr: $bb_stderr";++# The slot requested via --create-slot should now exist.+is( $node->safe_psql(+ 'postgres',+ "SELECT count(*) FROM pg_replication_slots WHERE slot_name = "+ . "'basebackup_race';"),+ '1',+ 'replication slot was created once pg_basebackup received the startpoint'+);++rmtree($backupdir);+$node->safe_psql('postgres',+ "SELECT pg_drop_replication_slot(slot_name) FROM pg_replication_slots "+ . "WHERE slot_name = 'basebackup_race';"+);++done_testing();--
2.50.1 (Apple Git-155)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nick.ivanov@enterprisedb.com, x4mmm@yandex-team.ru, alvherre@kurilemu.de, pgsql-hackers@lists.postgresql.org
Subject: Re: Possible race condition in pg_basebackup
In-Reply-To: <0cab2102-4786-416a-ae50-b606a495d01b@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox