Re: Return pg_control from pg_backup_stop().

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.

> diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
> index b4a0faccf41..93676057394 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 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.
> + </para>
> +

You shouldn't try to restore a backup_label file into an existing backup
that's missing it, so that wording sounds dangerous.

> <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/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
> index ac3d0c9328f..c4888c183fb 100644
> --- a/doc/src/sgml/ref/pg_rewind.sgml
> +++ b/doc/src/sgml/ref/pg_rewind.sgml
> @@ -455,6 +455,10 @@ GRANT EXECUTE ON FUNCTION pg_catalog.pg_read_binary_file(text, bigint, bigint, b
> 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>.
> </para>
> </step>
> <step>

That doesn't seem worth mentioning here; too much detail.

> diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
> index 20be788da78..56368a9378f 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.",

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?

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.

- Heikki