On 10/23/26 18:17, Heikki Linnakangas wrote:
On 23/10/2026 15:42, David Steele wrote:
Patch 02 implements the flag to enforce the presence of backup_label
and credits Andres with the original idea. This is the bulk of the
prior patch.
Makes sense at high level.
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1031,6 +1031,19 @@ SELECT * FROM pg_backup_stop(wait_for_archive
=> true);
vital to the backup working and must be written byte for byte
without
modification, which may require opening the file in binary mode.
</para>
+ <para>
+ Note that a backup made with the low-level API copies
+ <filename>pg_control</filename> from the running cluster, so it
does not
+ contain the flag that <xref linkend="app-pgbasebackup"/> sets to
make
+ <filename>backup_label</filename> mandatory for recovery. Removing
+ <filename>backup_label</filename> from such a backup will not
prevent the
+ cluster from starting; it will instead recover from the checkpoint
+ recorded in <filename>pg_control</filename>. Unless that happens
to be the
+ checkpoint the backup started from, recovery will begin too late
and
+ silently produce an inconsistent cluster.
+ Backup software using this API should set the flag itself if it
is able to
+ modify <filename>pg_control</filename>.
+ </para>
</listitem>
<listitem>
<para>
That's good advice, but the text feels awkward. Which flag do you need
to set exactly? And maybe the text should be placed elsewhere; the
copying of the pg_control file happened earlier already and you should
probably modify it already when you copy it.
On second thought I think this should just be removed rather than moved.
Modifying pg_control is not something we talk about in the docs, so
there are no good examples, and I don't think we should start here.
Removed in v18.
+ The copy of <filename>pg_control</filename> stored in the backup
is flagged
+ to require the <filename>backup_label</filename> file for
recovery. If
+ <filename>backup_label</filename> is removed, recovery refuses to
start
+ rather than silently producing an inconsistent cluster. Restoring
+ <filename>backup_label</filename> to the root of the backup allows
+ recovery to proceed, and the flag is cleared once recovery has
read the
+ file. <xref linkend="app-pgresetwal"/> also clears the flag, but
discarding
+ the WAL bypasses the replay that recovery needs, leaving an
inconsistent
+ cluster.
You shouldn't try to restore a backup_label file into an existing backup
that's missing it, so that wording sounds dangerous.
Agreed, I updated this in v18 to recommend a fresh restored including
backup label. Who knows where a user might get a backup_label from
otherwise?
I also updated the related hint to suggest a new restore:
- errhint("Restore the backup_label file that was created during the
backup."));
+ errhint("Restore the backup again, including the backup_label
file."));
defined as the result of <literal>pg_current_wal_insert_lsn()</
literal>
when rewinding from a live source or the last checkpoint LSN when
rewinding from a stopped source.
+ <filename>pg_control</filename> is also flagged to require
+ <filename>backup_label</filename>, so that removing that file
does not
+ allow the target to start up in an inconsistent state. The flag is
+ cleared once the target has processed <filename>backup_label</
filename>.
That doesn't seem worth mentioning here; too much detail.
Agreed. Removed in v18.
@@ -604,6 +604,10 @@ InitWalRecovery(ControlFileData *ControlFile,
bool *wasShutdown_ptr,
ereport(FATAL,
errmsg("could not find redo location %X/
%08X referenced by checkpoint record at %X/%08X",
LSN_FORMAT_ARGS(checkPoint.redo),
LSN_FORMAT_ARGS(CheckPointLoc)),
+ ControlFile->backupLabelRequired ?
+ errhint("Touch \"%s/recovery.signal\" or
\"%s/standby.signal\" and add required recovery options.\n"
+ "Do not remove \"%s/
backup_label\"; it is required to recover this backup.",
+ DataDir, DataDir, DataDir) :
errhint("If you are restoring from a
backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add
required recovery options.\n"
"If you are not restoring from a
backup, try removing the file \"%s/backup_label\".\n"
"Be careful: removing \"%s/
backup_label\" will result in a corrupt cluster if restoring from a
backup.",
Umm, not new with his patch, but why do we still have that hint to
remove backup_label? Is that ever the right thing to do? Why didn't we
remove that hint when exclusive-mode backups were removed?
Unfortunately I believe there is still a use case for this hint. When
making a backup, e.g. via snapshot, it is still easier to write the
backup_label into PGDATA, make the snapshot, then remove it. If the
cluster crashes before the removal then the answer is to remove the
backup label, just as it was if exclusive backup crashed.
I'm not excited about the above use case but not sure we can discount it
entirely.
What we could do is document that the above use case should be
accomplished by copying backup_label to another file in PGDATA, e.g.
backup_label.snap, and then renaming it when the snapshot is restored. I
think this is reasonable given that recovery.signal/standby.signal still
needed to be created so there is some work to be done on the restore
side. Maybe then it would be OK to remove the hint?
That feels like the subject for a separate patch, though. I have added
it to my notes.
Idea for further hardening: If you attempt to start up postgres and the
backup_label is present, set the flag in the control file. That way,
even if the backup tool doesn't modify the control file, it will be set
on first startup attempt. If you delete the backup_label later, you
still won't be able to start up into an inconsistent state. It won't
protect you if you remove the backup_label before attempting to start
up, but it'd catch some cases.
This would be dangerous if we accept that the above use case is valid.
In that case on restart the cluster would be bricked where in fact
removing the backup_label is the right thing to do.
Maybe we could set the flag when backup_label is present *and* either
standby.signal or recovery.signal. That works for the case above.
I originally had a patch that allowed backup software to get a non-torn
copy of pg_control with the flag set from pg_backup_stop(). If I can get
these patches committed I plan to reintroduce it. I think that is a
better answer for third-party software.
If we do decide to go with your idea instead (or in addition to), I'd
prefer to pursue it in a separate patch. I have added it to my notes.
Regards,
-David
From 463d1e1689a8a5fb1a70e75d0b2600027d1bbfe6 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Mon, 26 Oct 2026 11:31:05 +0000
Subject: Avoid a torn copy of pg_control in base backups.
Base backups read pg_control from disk without any protection against
concurrent writes by the server. At least ext4 and NTFS can expose partially
written contents to a concurrent read and a backup containing a torn pg_control
fails to start because the CRC does not match.
Read pg_control while holding ControlFileLock, as commit c558e6fd92f did for
the SQL functions that read pg_control. The file is read into memory and sent
from there so the lock is not held while the contents are streamed to the
client. Commit 63a582222c6 dealt with the same hazard in frontend tools by
retrying the read until the CRC is stable, but a base backup runs in the
server and can use ControlFileLock instead.
Since the read is done under the lock, a CRC mismatch means that pg_control
is actually corrupt so raise an error rather than include it in the backup.
Backups taken with the low-level API still copy pg_control from disk and are
not affected by this change.
Back-patch to all supported branches. Before v17, sendFileWithContent() gets
the length of the content with strlen(), which would truncate pg_control at
its first zero byte. Commit 7606175991f, which added the length argument, must
be back-patched first. It is confined to basebackup.c where the function is
static.
---
src/backend/backup/basebackup.c | 35 ++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index d7c485dc002..b5740f758dd 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -23,9 +23,11 @@
#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"
+#include "common/controldata_utils.h"
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "lib/stringinfo.h"
@@ -42,6 +44,7 @@
#include "storage/checksum.h"
#include "storage/dsm_impl.h"
#include "storage/ipc.h"
+#include "storage/lwlock.h"
#include "storage/reinit.h"
#include "utils/builtins.h"
#include "utils/guc.h"
@@ -340,9 +343,11 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
if (ti->path == NULL)
{
- struct stat statbuf;
bool sendtblspclinks = true;
char *backup_label;
+ ControlFileData *controlData;
+ bool crc_ok;
+ char
controlFile[PG_CONTROL_FILE_SIZE];
bbsink_begin_archive(sink, "base.tar");
@@ -364,15 +369,27 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
sendDir(sink, ".", 1, false, state.tablespaces,
sendtblspclinks, &manifest,
InvalidOid, ib);
- /* ... and pg_control after everything else. */
- if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
+ /*
+ * ... and pg_control after everything else.
Read it while
+ * holding ControlFileLock so that a concurrent
write cannot
+ * produce a torn copy.
+ */
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ controlData = get_controlfile(DataDir, &crc_ok);
+ LWLockRelease(ControlFileLock);
+
+ if (!crc_ok)
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);
+ (errmsg("calculated CRC
checksum does not match value stored in file")));
+
+ /* Send the full file, zero-padded as it is on
disk */
+ memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
+ memcpy(controlFile, controlData,
sizeof(ControlFileData));
+ pfree(controlData);
+
+ sendFileWithContent(sink, XLOG_CONTROL_FILE,
+
controlFile, PG_CONTROL_FILE_SIZE,
+
&manifest);
}
else
{
--
2.34.1
From 919b94a3367a260aa563f15bbd37683676749796 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Mon, 26 Oct 2026 11:31:05 +0000
Subject: 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.
Recovery hints that suggest removing backup_label are omitted when the flag is
set, since in that case pg_control shows that the data directory came from a
backup and removing backup_label would leave it unable to start at all.
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.
Andres Freund suggested this approach in an earlier thread that proposed
moving the contents of backup_label into pg_control.
Control and catalog version bumps are required.
Suggested-by: Andres Freund <[email protected]>
Discussion:
https://postgr.es/m/20231120203746.x3dgftmwvfgl5tdy%40awork3.anarazel.de
---
doc/src/sgml/backup.sgml | 11 ++
doc/src/sgml/func/func-info.sgml | 5 +
doc/src/sgml/ref/pg_basebackup.sgml | 12 ++
src/backend/access/transam/xlogrecovery.c | 27 ++-
src/backend/backup/basebackup.c | 8 +
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/bin/pg_rewind/t/RewindTest.pm | 12 ++
src/include/catalog/pg_control.h | 4 +
src/include/catalog/pg_proc.dat | 6 +-
src/test/recovery/meson.build | 1 +
src/test/recovery/t/042_low_level_backup.pl | 8 +
.../recovery/t/061_backup_label_required.pl | 165 ++++++++++++++++++
15 files changed, 264 insertions(+), 6 deletions(-)
create mode 100644 src/test/recovery/t/061_backup_label_required.pl
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 168444eccc5..5bba58ad8e8 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1031,6 +1031,17 @@ SELECT * FROM pg_backup_stop(wait_for_archive => true);
vital to the backup working and must be written byte for byte without
modification, which may require opening the file in binary mode.
</para>
+ <para>
+ Note that a backup made with the low-level API copies
+ <filename>pg_control</filename> from the running cluster, so it does not
+ contain the flag that <xref linkend="app-pgbasebackup"/> sets to make
+ <filename>backup_label</filename> mandatory for recovery. Removing
+ <filename>backup_label</filename> from such a backup will not prevent the
+ cluster from starting; it will instead recover from the checkpoint
+ recorded in <filename>pg_control</filename>. Unless that happens to be the
+ checkpoint the backup started from, recovery will begin too late and
+ silently produce an inconsistent cluster.
+ </para>
</listitem>
<listitem>
<para>
diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index c9c0261b52f..2f0651c694e 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3651,6 +3651,11 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
<entry><type>boolean</type></entry>
</row>
+ <row>
+ <entry><structfield>backup_label_required</structfield></entry>
+ <entry><type>boolean</type></entry>
+ </row>
+
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml
b/doc/src/sgml/ref/pg_basebackup.sgml
index b4a0faccf41..a94ee549524 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -60,6 +60,18 @@ PostgreSQL documentation
<xref linkend="app-pgdump"/> must be used.
</para>
+ <para>
+ The copy of <filename>pg_control</filename> stored in the backup is flagged
+ to require the <filename>backup_label</filename> file for recovery. If
+ <filename>backup_label</filename> is missing, recovery refuses to start
+ rather than silently producing an inconsistent cluster. The backup must
+ then be restored again, including <filename>backup_label</filename>. The
+ flag is cleared once recovery has read <filename>backup_label</filename>.
+ <xref linkend="app-pgresetwal"/> also clears the flag, but discarding the
+ WAL bypasses the replay that recovery needs, leaving an inconsistent
+ cluster.
+ </para>
+
<para>
The backup is made over a regular <productname>PostgreSQL</productname>
connection that uses the replication protocol. The connection must be made
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index 20be788da78..21cb1ce2797 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -604,6 +604,10 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
ereport(FATAL,
errmsg("could not find
redo location %X/%08X referenced by checkpoint record at %X/%08X",
LSN_FORMAT_ARGS(checkPoint.redo), LSN_FORMAT_ARGS(CheckPointLoc)),
+
ControlFile->backupLabelRequired ?
+ errhint("Touch
\"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery
options.\n"
+ "Do not
remove \"%s/backup_label\"; it is required to recover this backup.",
+
DataDir, DataDir, DataDir) :
errhint("If you are
restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\"
and add required recovery options.\n"
"If you
are not restoring from a backup, try removing the file \"%s/backup_label\".\n"
"Be
careful: removing \"%s/backup_label\" will result in a corrupt cluster if
restoring from a backup.",
@@ -615,6 +619,10 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
ereport(FATAL,
errmsg("could not locate required
checkpoint record at %X/%08X",
LSN_FORMAT_ARGS(CheckPointLoc)),
+ ControlFile->backupLabelRequired ?
+ errhint("Touch \"%s/recovery.signal\"
or \"%s/standby.signal\" and add required recovery options.\n"
+ "Do not remove
\"%s/backup_label\"; it is required to recover this backup.",
+ DataDir, DataDir,
DataDir) :
errhint("If you are restoring from a
backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required
recovery options.\n"
"If you are not
restoring from a backup, try removing the file \"%s/backup_label\".\n"
"Be careful: removing
\"%s/backup_label\" will result in a corrupt cluster if restoring from a
backup.",
@@ -659,7 +667,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 again,
including the backup_label file."));
/*
* If tablespace_map file is present without backup_label file,
there
@@ -939,11 +954,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 b5740f758dd..688f65c9405 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -382,6 +382,14 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
ereport(ERROR,
(errmsg("calculated CRC
checksum does not match value stored in file")));
+ /* Require backup_label for recovery from this
backup */
+ controlData->backupLabelRequired = true;
+
+ INIT_CRC32C(controlData->crc);
+ COMP_CRC32C(controlData->crc, controlData,
+
offsetof(ControlFileData, crc));
+ FIN_CRC32C(controlData->crc);
+
/* Send the full file, zero-padded as it is on
disk */
memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
memcpy(controlFile, controlData,
sizeof(ControlFileData));
diff --git a/src/backend/utils/misc/pg_controldata.c
b/src/backend/utils/misc/pg_controldata.c
index 1f47302fe2a..232c888d594 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 6a0f848d8d0..8bcca89733b 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 634d966da9e..1b9846fb8a4 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -924,6 +924,7 @@ RewriteControlFile(void)
ControlFile.backupStartPoint = InvalidXLogRecPtr;
ControlFile.backupEndPoint = InvalidXLogRecPtr;
ControlFile.backupEndRequired = false;
+ ControlFile.backupLabelRequired = false;
/*
* The old WAL is gone and the new position may lie below the old
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index d2521dab333..a16081669e1 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -783,6 +783,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;
/*
* Keep the target's own data checksum state. Most of the data
directory
diff --git a/src/bin/pg_rewind/t/RewindTest.pm
b/src/bin/pg_rewind/t/RewindTest.pm
index 32aeca80f13..b2a7fca9d02 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -353,6 +353,18 @@ sub run_pg_rewind
croak("Incorrect test mode specified");
}
+ # pg_rewind writes a backup_label of its own when it rewinds, and the
+ # target must then require it for recovery; the flag is cleared again
when
+ # the target is restarted below and recovery consumes backup_label.
When
+ # the target is already an ancestor of the source no rewind takes place,
+ # and pg_control must be left alone so that the target still starts.
+ my $label_required =
+ -f "$primary_pgdata/backup_label" ? 'yes' : 'no';
+ command_like(
+ [ 'pg_controldata', '--pgdata' => $primary_pgdata ],
+ qr/Backup label required: +$label_required/,
+ 'backup_label requirement matches whether a rewind took place');
+
# Now move back postgresql.conf with old settings
move(
"$tmp_folder/primary-postgresql.conf.tmp",
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index c3c934d0012..f83928e2ca3 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 1f21a8eb6d4..a885c173c25 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12443,9 +12443,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 b6ac9dd07a7..31b0cbd34e1 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -69,6 +69,7 @@ tests += {
't/058_shutdown_crash_restart.pl',
't/059_remote_apply_status_interval.pl',
't/060_multixact_crashed_updater.pl',
+ 't/061_backup_label_required.pl',
],
},
}
diff --git a/src/test/recovery/t/042_low_level_backup.pl
b/src/test/recovery/t/042_low_level_backup.pl
index 7ed54e611eb..8633b690cc6 100644
--- a/src/test/recovery/t/042_low_level_backup.pl
+++ b/src/test/recovery/t/042_low_level_backup.pl
@@ -165,4 +165,12 @@ ok( $node_replica->log_contains(
'FATAL: .*could not locate required checkpoint record at'),
'ends with FATAL for missing required checkpoint record');
+# A low-level backup copies pg_control from the running cluster, so it does not
+# require backup_label and the hint still offers removing the file as a way to
+# recover a cluster that is not being restored from a backup. Backups made
+# with pg_basebackup do require it and get a different hint; see
+# 061_backup_label_required.pl.
+ok($node_replica->log_contains('try removing the file .*backup_label'),
+ 'hint offers removing backup_label when it is not required');
+
done_testing();
diff --git a/src/test/recovery/t/061_backup_label_required.pl
b/src/test/recovery/t/061_backup_label_required.pl
new file mode 100644
index 00000000000..5d1cee27054
--- /dev/null
+++ b/src/test/recovery/t/061_backup_label_required.pl
@@ -0,0 +1,165 @@
+# 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::RecursiveCopy;
+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);
+
+# Set wal_keep_size to prevent WAL segment recycling after the checkpoints
+# enforced by the backups below. The standby is created from the first backup
+# but started only after later backups have advanced the primary, so it must
+# still be able to stream the segments in between. This is set before the
+# first backup so that it is inherited by the standbys.
+$node_primary->append_conf('postgresql.conf', 'wal_keep_size = 64MB');
+$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 not set on the source cluster');
+
+# pg_resetwal clears the flag. This is not ideal since it leaves an
inconsistent
+# cluster, but at least verify that the flag is cleared.
+my $reset_dir = $node_primary->backup_dir . '/' . $backup_name . '_reset';
+PostgreSQL::Test::RecursiveCopy::copypath(
+ $node_primary->backup_dir . '/' . $backup_name, $reset_dir);
+chmod(0700, $reset_dir) or BAIL_OUT("could not chmod $reset_dir");
+
+command_ok([ 'pg_resetwal', '--force', '--pgdata' => $reset_dir ],
+ 'pg_resetwal runs on a backup that requires backup_label');
+command_like(
+ [ 'pg_controldata', '--pgdata' => $reset_dir ],
+ qr/Backup label required: +no/,
+ 'pg_resetwal clears 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 made without WAL cannot reach a consistent state on its own, and
+# recovery reports that it could not locate the checkpoint record. Since
+# pg_control requires backup_label, the hint for that error must not repeat the
+# usual advice to remove the file.
+my $nowal_backup = 'backup_nowal';
+$node_primary->backup($nowal_backup,
+ backup_options => [ '--wal-method' => 'none' ]);
+
+my $node_nowal = PostgreSQL::Test::Cluster->new('nowal');
+$node_nowal->init_from_backup($node_primary, $nowal_backup);
+
+my $nowal_offset = -s $node_nowal->logfile;
+is($node_nowal->start(fail_ok => 1), 0,
+ 'backup without WAL fails to start when no recovery options are set');
+ok( $node_nowal->log_contains(
+ 'FATAL: .*could not locate required checkpoint record',
$nowal_offset),
+ 'missing checkpoint record is reported');
+ok( $node_nowal->log_contains('HINT: .*recovery\.signal', $nowal_offset),
+ 'hint points at the recovery signal files');
+ok( $node_nowal->log_contains('Do not remove .*backup_label', $nowal_offset),
+ 'hint tells the user to keep backup_label');
+ok( !$node_nowal->log_contains('try removing the file', $nowal_offset),
+ 'hint does not suggest removing backup_label');
+
+# 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();
--
2.34.1
