From 8d6a26b5d271bab4627d3be28ddaf28536cb7a62 Mon Sep 17 00:00:00 2001 From: Nick Ivanov 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.c index 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.c index 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.c index 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.txt index 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.dat index 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.sample index 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.h index 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.h index 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.h index 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.build index 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.pl new file mode 100644 index 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)