From ab7cfdc1fa59f57db5994a6a7000e74af4be1288 Mon Sep 17 00:00:00 2001 From: David Steele Date: Thu, 27 Aug 2026 22:13:22 -0400 Subject: [PATCH] Add pg_control flag to prevent recovery without backup_label. Harden recovery by adding a flag to pg_control to indicate that backup_label is required. This prevents the user from deleting backup_label resulting in an inconsistent recovery. Another advantage is that the copy of pg_control used by pg_basebackup is guaranteed not to be torn. This functionality is limited to pg_basebackup and pg_rewind (or any software comfortable with modifying pg_control). pg_rewind creates a backup_label of its own, so the flag is set there as well, which means that removing backup_label after a rewind no longer allows the cluster to start. Control and catalog version bumps are required. --- doc/src/sgml/func/func-info.sgml | 5 + src/backend/access/transam/xlog.c | 44 +++++++ src/backend/access/transam/xlogrecovery.c | 19 ++- src/backend/backup/basebackup.c | 15 +-- src/backend/utils/misc/pg_controldata.c | 7 +- src/bin/pg_controldata/pg_controldata.c | 2 + src/bin/pg_resetwal/pg_resetwal.c | 1 + src/bin/pg_rewind/pg_rewind.c | 1 + src/include/access/xlog.h | 1 + src/include/catalog/pg_control.h | 4 + src/include/catalog/pg_proc.dat | 6 +- src/test/recovery/meson.build | 1 + .../recovery/t/057_backup_label_required.pl | 119 ++++++++++++++++++ 13 files changed, 210 insertions(+), 15 deletions(-) create mode 100644 src/test/recovery/t/057_backup_label_required.pl diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml index 2f03766b67a..afbbbcab20e 100644 --- a/doc/src/sgml/func/func-info.sgml +++ b/doc/src/sgml/func/func-info.sgml @@ -3659,6 +3659,11 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} boolean + + backup_label_required + boolean + + diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index de4c96e135f..a704091e080 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -10139,6 +10139,50 @@ do_pg_abort_backup(int code, Datum arg) } } +/* + * Create a consistent copy of control data to be used for backup and update it + * to require a backup label for recovery. Also recalculate the CRC. + * + * All field access is done through a local, properly-aligned ControlFileData; + * the caller's buffer is only ever written via memcpy() and so need not be + * aligned for ControlFileData (e.g. it may point into the payload of a bytea). + */ +void +backup_control_file(uint8 *controlFile) +{ + ControlFileData controlData; + + LWLockAcquire(ControlFileLock, LW_SHARED); + memcpy(&controlData, ControlFile, sizeof(ControlFileData)); + +#ifdef USE_ASSERT_CHECKING + /* + * Verify that the contents of pg_control are the same in memory as on disk + */ + { + bool crc_ok; + ControlFileData *dataDisk = get_controlfile(DataDir, &crc_ok); + + Assert(crc_ok && + memcmp(dataDisk, &controlData, sizeof(ControlFileData)) == 0); + + pfree(dataDisk); + } +#endif + + LWLockRelease(ControlFileLock); + + controlData.backupLabelRequired = true; + + INIT_CRC32C(controlData.crc); + COMP_CRC32C(controlData.crc, &controlData, offsetof(ControlFileData, crc)); + FIN_CRC32C(controlData.crc); + + /* Copy into the caller's buffer, zero-padded to the full file size */ + memset(controlFile, 0, PG_CONTROL_FILE_SIZE); + memcpy(controlFile, &controlData, sizeof(ControlFileData)); +} + /* * Register a handler that will warn about unterminated backups at end of * session, unless this has already been done. diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index 6de13b91748..dc7198da248 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -659,7 +659,14 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, } else { - /* No backup_label file has been found if we are here. */ + /* + * No backup_label file has been found if we are here. Error if the + * control file requires backup_label. + */ + if (ControlFile->backupLabelRequired) + ereport(FATAL, + errmsg("could not find backup_label required for recovery"), + errhint("Restore the backup_label file that was created during the backup.")); /* * If tablespace_map file is present without backup_label file, there @@ -939,11 +946,21 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, * * Any other state indicates that the backup somehow became corrupted * and we can't sensibly continue with recovery. + * + * backupLabelRequired is set to false since backup_label is no longer + * required once pg_control has been updated on disk. If recovery + * terminates abnormally between when pg_control is updated and + * backup_label is renamed then on restart pg_control will be + * reinitialized from backup_label. If the user manually deletes + * backup_label before restarting then recovery will proceed with the + * contents of pg_control just as it would if the crash had happened + * directly after backup_label rename. */ if (haveBackupLabel) { ControlFile->backupStartPoint = checkPoint.redo; ControlFile->backupEndRequired = backupEndRequired; + ControlFile->backupLabelRequired = false; if (backupFromStandby) { diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index e3c04ecd810..74ab7781512 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -23,6 +23,7 @@ #include "backup/basebackup_incremental.h" #include "backup/basebackup_sink.h" #include "backup/basebackup_target.h" +#include "catalog/pg_control.h" #include "catalog/pg_tablespace_d.h" #include "commands/defrem.h" #include "common/compression.h" @@ -340,9 +341,9 @@ perform_base_backup(basebackup_options *opt, bbsink *sink, if (ti->path == NULL) { - struct stat statbuf; bool sendtblspclinks = true; char *backup_label; + uint8 controlFile[PG_CONTROL_FILE_SIZE]; bbsink_begin_archive(sink, "base.tar"); @@ -365,14 +366,10 @@ perform_base_backup(basebackup_options *opt, bbsink *sink, sendtblspclinks, &manifest, InvalidOid, ib); /* ... and pg_control after everything else. */ - if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not stat file \"%s\": %m", - XLOG_CONTROL_FILE))); - sendFile(sink, XLOG_CONTROL_FILE, XLOG_CONTROL_FILE, &statbuf, - false, InvalidOid, InvalidOid, - InvalidRelFileNumber, 0, &manifest, 0, NULL, 0); + backup_control_file(controlFile); + sendFileWithContent(sink, XLOG_CONTROL_FILE, + (char *) controlFile, PG_CONTROL_FILE_SIZE, + &manifest); } else { diff --git a/src/backend/utils/misc/pg_controldata.c b/src/backend/utils/misc/pg_controldata.c index 9014f0953e9..e328fcc44ff 100644 --- a/src/backend/utils/misc/pg_controldata.c +++ b/src/backend/utils/misc/pg_controldata.c @@ -168,8 +168,8 @@ pg_control_checkpoint(PG_FUNCTION_ARGS) Datum pg_control_recovery(PG_FUNCTION_ARGS) { - Datum values[5]; - bool nulls[5]; + Datum values[6]; + bool nulls[6]; TupleDesc tupdesc; HeapTuple htup; ControlFileData *ControlFile; @@ -201,6 +201,9 @@ pg_control_recovery(PG_FUNCTION_ARGS) values[4] = BoolGetDatum(ControlFile->backupEndRequired); nulls[4] = false; + values[5] = BoolGetDatum(ControlFile->backupLabelRequired); + nulls[5] = false; + htup = heap_form_tuple(tupdesc, values, nulls); PG_RETURN_DATUM(HeapTupleGetDatum(htup)); diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c index 6fc87ed114d..200b9777e06 100644 --- a/src/bin/pg_controldata/pg_controldata.c +++ b/src/bin/pg_controldata/pg_controldata.c @@ -305,6 +305,8 @@ main(int argc, char *argv[]) LSN_FORMAT_ARGS(ControlFile->backupEndPoint)); printf(_("End-of-backup record required: %s\n"), ControlFile->backupEndRequired ? _("yes") : _("no")); + printf(_("Backup label required: %s\n"), + ControlFile->backupLabelRequired ? _("yes") : _("no")); printf(_("wal_level setting: %s\n"), wal_level_str(ControlFile->wal_level)); printf(_("wal_log_hints setting: %s\n"), diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c index 1542a56ca4b..7b5e71f4987 100644 --- a/src/bin/pg_resetwal/pg_resetwal.c +++ b/src/bin/pg_resetwal/pg_resetwal.c @@ -922,6 +922,7 @@ RewriteControlFile(void) ControlFile.backupStartPoint = InvalidXLogRecPtr; ControlFile.backupEndPoint = InvalidXLogRecPtr; ControlFile.backupEndRequired = false; + ControlFile.backupLabelRequired = false; /* * Force the defaults for max_* settings. The values don't really matter diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index 2e86fd158d0..f3f1b15310d 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -738,6 +738,7 @@ perform_rewind(filemap_t *filemap, rewind_source *source, ControlFile_new.minRecoveryPoint = endrec; ControlFile_new.minRecoveryPointTLI = endtli; ControlFile_new.state = DB_IN_ARCHIVE_RECOVERY; + ControlFile_new.backupLabelRequired = true; if (!dry_run) update_controlfile(datadir_target, &ControlFile_new, do_sync); } diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 130ba929109..340387e7042 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -340,6 +340,7 @@ extern void do_pg_backup_start(const char *backupidstr, bool fast, StringInfo tblspcmapfile); extern void do_pg_backup_stop(BackupState *state, bool waitforarchive); extern void do_pg_abort_backup(int code, Datum arg); +extern void backup_control_file(uint8 *controlFile); extern void register_persistent_abort_backup_handler(void); extern SessionBackupState get_backup_status(void); diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h index 7b5404460ec..c298e247a21 100644 --- a/src/include/catalog/pg_control.h +++ b/src/include/catalog/pg_control.h @@ -172,12 +172,16 @@ typedef struct ControlFileData * If backupEndRequired is true, we know for sure that we're restoring * from a backup, and must see a backup-end record before we can safely * start up. + * + * If backupLabelRequired is true, then a backup_label file must be + * present in order for recovery to proceed. */ XLogRecPtr minRecoveryPoint; TimeLineID minRecoveryPointTLI; XLogRecPtr backupStartPoint; XLogRecPtr backupEndPoint; bool backupEndRequired; + bool backupLabelRequired; /* * Parameter settings that determine if the WAL can be used for archival diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 6979c7d1161..ca0b852d958 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -12440,9 +12440,9 @@ { oid => '3443', descr => 'pg_controldata recovery state information as a function', proname => 'pg_control_recovery', provolatile => 'v', prorettype => 'record', - proargtypes => '', proallargtypes => '{pg_lsn,int4,pg_lsn,pg_lsn,bool}', - proargmodes => '{o,o,o,o,o}', - proargnames => '{min_recovery_end_lsn,min_recovery_end_timeline,backup_start_lsn,backup_end_lsn,end_of_backup_record_required}', + proargtypes => '', proallargtypes => '{pg_lsn,int4,pg_lsn,pg_lsn,bool,bool}', + proargmodes => '{o,o,o,o,o,o}', + proargnames => '{min_recovery_end_lsn,min_recovery_end_timeline,backup_start_lsn,backup_end_lsn,end_of_backup_record_required,backup_label_required}', prosrc => 'pg_control_recovery' }, { oid => '3444', diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index 72113c5ac6e..082dfb17c00 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_backup_label_required.pl', ], }, } diff --git a/src/test/recovery/t/057_backup_label_required.pl b/src/test/recovery/t/057_backup_label_required.pl new file mode 100644 index 00000000000..79e3a352e92 --- /dev/null +++ b/src/test/recovery/t/057_backup_label_required.pl @@ -0,0 +1,119 @@ +# Copyright (c) 2021-2026, PostgreSQL Global Development Group + +# Test the pg_control flag that makes backup_label mandatory for recovery. +# +# pg_basebackup stores a modified copy of pg_control in the backup, with a flag +# set that makes recovery refuse to start if backup_label is missing. This +# prevents the silent corruption that results from removing the file, both for +# backups taken from a primary and from a standby. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Rename backup_label in the data directory of the given node, start it, and +# check that recovery refuses to proceed. Then put the file back. +sub check_startup_without_backup_label +{ + my ($node, $test_name) = @_; + my $data_dir = $node->data_dir; + my $log_offset = -s $node->logfile; + + rename("$data_dir/backup_label", "$data_dir/backup_label.tmp") + or BAIL_OUT("could not rename $data_dir/backup_label"); + + is($node->start(fail_ok => 1), 0, "$test_name: startup fails"); + ok( $node->log_contains( + 'FATAL: .*could not find backup_label required for recovery', + $log_offset), + "$test_name: ends with FATAL for missing backup_label"); + + rename("$data_dir/backup_label.tmp", "$data_dir/backup_label") + or BAIL_OUT("could not rename $data_dir/backup_label.tmp"); + return; +} + +my $node_primary = PostgreSQL::Test::Cluster->new('primary'); +$node_primary->init(allows_streaming => 1); +$node_primary->start; + +$node_primary->safe_psql('postgres', + 'CREATE TABLE tab_int AS SELECT generate_series(1, 1000) AS a'); + +# Take a backup from the primary. The copy of pg_control stored in the backup +# must require backup_label, while the control file of the running cluster is +# left alone. +my $backup_name = 'backup_primary'; +$node_primary->backup($backup_name); + +command_like( + [ + 'pg_controldata', + '--pgdata' => $node_primary->backup_dir . '/' . $backup_name + ], + qr/Backup label required: +yes/, + 'backup taken from a primary requires backup_label'); +command_like( + [ 'pg_controldata', '--pgdata' => $node_primary->data_dir ], + qr/Backup label required: +no/, + 'control file of the source cluster is unchanged'); +is( $node_primary->safe_psql( + 'postgres', 'SELECT backup_label_required FROM pg_control_recovery()'), + 'f', + 'pg_control_recovery() reports the flag'); + +# Restoring that backup without backup_label must not start. +my $node_restored = PostgreSQL::Test::Cluster->new('restored'); +$node_restored->init_from_backup($node_primary, $backup_name); + +check_startup_without_backup_label($node_restored, 'backup from primary'); + +# With backup_label back in place recovery completes, and the flag is cleared +# so that subsequent restarts no longer need the file. +$node_restored->start; +is($node_restored->safe_psql('postgres', 'SELECT count(*) FROM tab_int'), + 1000, 'restored cluster has the expected contents'); +is( $node_restored->safe_psql( + 'postgres', 'SELECT backup_label_required FROM pg_control_recovery()'), + 'f', + 'flag is cleared once recovery has completed'); +$node_restored->stop; + +command_like( + [ 'pg_controldata', '--pgdata' => $node_restored->data_dir ], + qr/Backup label required: +no/, + 'control file no longer requires backup_label after recovery'); + +# A backup taken from a standby gets the same treatment. This is the case that +# previously required backup software to copy pg_control last. +my $node_standby = PostgreSQL::Test::Cluster->new('standby'); +$node_standby->init_from_backup($node_primary, $backup_name, + has_streaming => 1); +$node_standby->start; +$node_primary->wait_for_replay_catchup($node_standby); + +my $standby_backup = 'backup_standby'; +$node_standby->backup($standby_backup); + +command_like( + [ + 'pg_controldata', + '--pgdata' => $node_standby->backup_dir . '/' . $standby_backup + ], + qr/Backup label required: +yes/, + 'backup taken from a standby requires backup_label'); + +my $node_standby2 = PostgreSQL::Test::Cluster->new('standby2'); +$node_standby2->init_from_backup($node_standby, $standby_backup, + has_streaming => 1); + +check_startup_without_backup_label($node_standby2, 'backup from standby'); + +$node_standby2->start; +$node_standby->wait_for_replay_catchup($node_standby2, $node_primary); +is($node_standby2->safe_psql('postgres', 'SELECT count(*) FROM tab_int'), + 1000, 'cascading standby from a standby backup is caught up'); + +done_testing();