From 05d05bb10767cacb6e9f170c35bbbe62b10c4cc8 Mon Sep 17 00:00:00 2001 From: Nick Ivanov Date: Fri, 28 Aug 2026 17:58:11 +0100 Subject: [PATCH v1] 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 --- src/backend/access/transam/xlog.c | 23 +++ src/backend/access/transam/xlogbackup.c | 148 ++++++++++++++++++ src/backend/backup/basebackup.c | 6 + .../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/056_basebackup_slot_race.pl | 135 ++++++++++++++++ 11 files changed, 366 insertions(+) create mode 100644 src/test/recovery/t/056_basebackup_slot_race.pl diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index de4c96e135f..744f1b3dcd3 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" @@ -8536,6 +8537,7 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo) XLogSegNo currSegNo; XLogSegNo segno; XLogRecPtr keep; + XLogRecPtr keep_for_backups; XLByteToSeg(recptr, currSegNo, wal_segment_size); segno = currSegNo; @@ -8543,9 +8545,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 @@ -9694,6 +9709,8 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces, WALInsertLockRelease(); } while (!gotUniqueStartpoint); + RegisterBackupStartpoint(state->startpoint); + /* * Construct tablespace_map file. */ @@ -9898,6 +9915,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. @@ -10133,6 +10153,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..656d4f3aeab 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -324,6 +324,12 @@ perform_base_backup(basebackup_options *opt, bbsink *sink, state.bytes_total_is_valid = true; } + /* + * 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); + /* notify basebackup sink about start of backup */ bbsink_begin_backup(sink, &state, SINK_BUFFER_LENGTH); diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt index 256b3a3c02e..54a13e47782 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 39ec8c4946d..b61bc60c000 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -64,6 +64,7 @@ tests += { 't/053_standby_login_event_trigger.pl', 't/054_unlogged_sequence_promotion.pl', 't/055_cascade_reconnect.pl', + 't/056_basebackup_slot_race.pl', ], }, } diff --git a/src/test/recovery/t/056_basebackup_slot_race.pl b/src/test/recovery/t/056_basebackup_slot_race.pl new file mode 100644 index 00000000000..2e83f825cae --- /dev/null +++ b/src/test/recovery/t/056_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)