Hi Corey,

On 3/19/26 04:00, Corey Huinker wrote:
    Whatever gets committed for PG19 I'll write a followup patch to
    describe
    the hazards of reading pg_control and generally how to get a good copy.
    However, this will be complicated enough that the best answer will
    likely be to use pg_basebackup or some other reputable backup software.
    I don't love this -- I feel like the low-level interface should be
    usable with such hazards.

Surya Poondla and I had decided on this patchset as a pair-reviewing exercise. However, events have overtaken us, and several other people have chimed in expressing the same concerns that we had observed but hadn't yet completed our review.

Thank you both for having a look!

> All of the main concerns that we had > found up to this point have been addressed in the lastest patchset,
except for the trivial observation that the ereport() uses the old style and doesn't need the set of parens around (errmsg(), errhint()).

Grep shows there are lots of messages with the new style but many more in the old style. Presumably they are only being updated as they are modified. Do you happen to know the commit or message thread where this policy was started? I've been searching but it is such a generic search term.

I've updated the message style in the new patches.

Patches apply clean, tests pass, test coverage seems sufficient, we're happy with the wording of the documentation, in short there really isn't a whole lot for us to add to the review, and for that reason we're removing our names from the list of reviewers in the commitfest app.

It seems to me you've still done a review. Confirming what the other reviewers found is good info to have.

Regards,
-David
From 79f696a8936b05849bf38a5d393d93c642a2450b Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Thu, 19 Mar 2026 02:50:51 +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.

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 (or any software comfortable
with modifying pg_control).

Control and catalog version bumps are required.
---
 doc/src/sgml/func/func-info.sgml          |  5 ++++
 src/backend/access/transam/xlog.c         | 36 +++++++++++++++++++++++
 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/t/002_archiving.pl      | 20 +++++++++++++
 12 files changed, 102 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index 5b5f1f3c5df..b1b49ce7b07 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3661,6 +3661,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/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index f5c9a34374d..87ee245e3f4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9592,6 +9592,42 @@ 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.
+ */
+void
+backup_control_file(uint8_t *controlFile)
+{
+       ControlFileData *controlData = ((ControlFileData *)controlFile);
+
+       memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
+
+       LWLockAcquire(ControlFileLock, LW_SHARED);
+       memcpy(controlFile, 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, controlFile, 
sizeof(ControlFileData)) == 0);
+       }
+#endif
+
+       LWLockRelease(ControlFileLock);
+
+       controlData->backupLabelRequired = true;
+
+       INIT_CRC32C(controlData->crc);
+       COMP_CRC32C(controlData->crc, controlFile, offsetof(ControlFileData, 
crc));
+       FIN_CRC32C(controlData->crc);
+}
+
 /*
  * 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 6d2c4a86b96..e2810d68a0a 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -647,7 +647,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("backup_label must be present 
for recovery to proceed"));
 
                /*
                 * If tablespace_map file is present without backup_label file, 
there
@@ -927,11 +934,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 ab1fbae8001..c4f7827f866 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"
@@ -332,9 +333,9 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
 
                        if (ti->path == NULL)
                        {
-                               struct stat statbuf;
                                bool            sendtblspclinks = true;
                                char       *backup_label;
+                               uint8_t controlFile[PG_CONTROL_FILE_SIZE];
 
                                bbsink_begin_archive(sink, "base.tar");
 
@@ -357,14 +358,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 c6d9cbb1577..c2c19eb77df 100644
--- a/src/backend/utils/misc/pg_controldata.c
+++ b/src/backend/utils/misc/pg_controldata.c
@@ -162,8 +162,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;
@@ -195,6 +195,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 a4060309ae0..5919dd58fed 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -301,6 +301,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 ab766c34d4b..768a4ed2f18 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -918,6 +918,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 9d745d4b25b..509b9e80a21 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -736,6 +736,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 dcc12eb8cbe..469739aa23f 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -313,6 +313,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_t *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 77a661e818b..d9841f58108 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -166,12 +166,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 fc8d82665b8..999ea2ac801 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12504,9 +12504,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/t/002_archiving.pl 
b/src/test/recovery/t/002_archiving.pl
index aa40f58e6d6..9963d13473e 100644
--- a/src/test/recovery/t/002_archiving.pl
+++ b/src/test/recovery/t/002_archiving.pl
@@ -41,6 +41,26 @@ $node_standby->append_conf(
 archive_cleanup_command = 'echo archive_cleanup_done > 
$archive_cleanup_command_file'
 recovery_end_command = 'echo recovery_ended_done > $recovery_end_command_file'
 ));
+
+# Rename backup_label to verify that recovery will not start without it
+rename("$data_dir/backup_label", "$data_dir/backup_label.tmp")
+  or BAIL_OUT "could not move $data_dir/backup_label";
+
+my $res = run_log(
+       [
+               'pg_ctl', '-D', $node_standby->data_dir, '-l',
+               $node_standby->logfile, 'start'
+       ]);
+ok(!$res, 'invalid recovery startup fails');
+
+my $logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/could not find backup_label required for recovery/,
+       'could not find backup_label required for recovery');
+
+# Restore backup_label so recovery proceeds normally
+rename("$data_dir/backup_label.tmp", "$data_dir/backup_label")
+  or BAIL_OUT "could not move $data_dir/backup_label";
+
 $node_standby->start;
 
 # Create some content on primary
-- 
2.34.1

From b7ed57808d81a2d1fcc09487fb7a566a515f2351 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Thu, 19 Mar 2026 02:50:52 +0000
Subject: Return pg_control from pg_backup_stop().

Harden recovery by returning a copy of pg_control from pg_backup_stop() that has
a flag set to prevent recovery if the backup_label file is missing. Instead of
backup software copying pg_control from PGDATA, it stores an updated version
that is returned from pg_backup_stop(). This is better for the following
reasons:

* The user can no longer remove backup_label and get what looks like a
successful recovery (while almost certainly causing corruption). If backup_label
is removed the cluster will not start. The user may try pg_resetwal, but that
tool makes it pretty clear that corruption will result from its use.

* We don't need to worry about backup software seeing a torn copy of pg_control,
since Postgres can safely read it out of memory and provide a valid copy via
pg_backup_stop(). This solves torn reads without needing to write pg_control via
a temp file, which may affect performance on a standby.

* For backup from standby, we no longer need to instruct the backup software to
copy pg_control last. In fact the backup software should not copy pg_control 
from
PGDATA at all.

These changes have no impact on current backup software and they are free to use
the pg_control available from pg_stop_backup() or continue to use pg_control 
from
PGDATA. Of course they will miss the benefits of getting a consistent copy of
pg_control and the backup_label checking, but will be no worse off than before.

Catalog version bump is required.
---
 doc/src/sgml/backup.sgml                    | 18 +++++-
 src/backend/access/transam/xlogfuncs.c      | 20 ++++--
 src/include/catalog/pg_proc.dat             |  4 +-
 src/test/recovery/t/042_low_level_backup.pl | 67 ++++++++++++++++++++-
 4 files changed, 97 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 168444eccc5..67974b7b1b6 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1027,9 +1027,12 @@ SELECT * FROM pg_backup_stop(wait_for_archive => true);
      values. The second of these fields should be written to a file named
      <filename>backup_label</filename> in the root directory of the backup. The
      third field should be written to a file named
-     <filename>tablespace_map</filename> unless the field is empty. These 
files are
+     <filename>tablespace_map</filename> unless the field is empty. The fourth
+     field should be written into a file named
+     <filename>global/pg_control</filename> (overwriting the existing file when
+     present). These files are
      vital to the backup working and must be written byte for byte without
-     modification, which may require opening the file in binary mode.
+     modification, which will require opening the file in binary mode.
     </para>
    </listitem>
    <listitem>
@@ -1101,7 +1104,16 @@ SELECT * FROM pg_backup_stop(wait_for_archive => true);
    </para>
 
    <para>
-    You should, however, omit from the backup the files within the
+    You should exclude <filename>global/pg_control</filename> from your backup
+    and put the contents of the <parameter>controlfile</parameter> column
+    returned from <function>pg_backup_stop</function> in your backup at
+    <filename>global/pg_control</filename>. This version of pg_control contains
+    safeguards against recovery without backup_label present and is guaranteed
+    not to be torn.
+   </para>
+
+   <para>
+    You should also omit from the backup the files within the
     cluster's <filename>pg_wal/</filename> subdirectory.  This
     slight adjustment is worthwhile because it reduces the risk
     of mistakes when restoring.  This is easy to arrange if
diff --git a/src/backend/access/transam/xlogfuncs.c 
b/src/backend/access/transam/xlogfuncs.c
index 4e35311b2f3..fab0c047621 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -143,9 +143,11 @@ pg_backup_start(PG_FUNCTION_ARGS)
  *
  * The backup_label contains the user-supplied label string (typically this
  * would be used to tell where the backup dump will be stored), the starting
- * time, starting WAL location for the dump and so on.  It is the caller's
- * responsibility to write the backup_label and tablespace_map files in the
- * data folder that will be restored from this backup.
+ * time, starting WAL location for the dump and so on.  The pg_control file
+ * contains a consistent copy of pg_control that also has a safeguard against
+ * being used without backup_label.  It is the caller's responsibility to write
+ * the backup_label, pg_control, and tablespace_map files in the data folder
+ * that will be restored from this backup.
  *
  * Permission checking for this function is managed through the normal
  * GRANT system.
@@ -153,12 +155,14 @@ pg_backup_start(PG_FUNCTION_ARGS)
 Datum
 pg_backup_stop(PG_FUNCTION_ARGS)
 {
-#define PG_BACKUP_STOP_V2_COLS 3
+#define PG_BACKUP_STOP_V2_COLS 4
        TupleDesc       tupdesc;
        Datum           values[PG_BACKUP_STOP_V2_COLS] = {0};
        bool            nulls[PG_BACKUP_STOP_V2_COLS] = {0};
        bool            waitforarchive = PG_GETARG_BOOL(0);
        char       *backup_label;
+       bytea      *pg_control_bytea;
+       uint8_t         pg_control[PG_CONTROL_FILE_SIZE];
        SessionBackupState status = get_backup_status();
 
        /* Initialize attributes information in the tuple descriptor */
@@ -174,6 +178,13 @@ pg_backup_stop(PG_FUNCTION_ARGS)
        Assert(backup_state != NULL);
        Assert(tablespace_map != NULL);
 
+       /* Build the contents of pg_control */
+       backup_control_file(pg_control);
+
+       pg_control_bytea = (bytea *) palloc(PG_CONTROL_FILE_SIZE + VARHDRSZ);
+       SET_VARSIZE(pg_control_bytea, PG_CONTROL_FILE_SIZE + VARHDRSZ);
+       memcpy(VARDATA(pg_control_bytea), pg_control, PG_CONTROL_FILE_SIZE);
+
        /* Stop the backup */
        do_pg_backup_stop(backup_state, waitforarchive);
 
@@ -183,6 +194,7 @@ pg_backup_stop(PG_FUNCTION_ARGS)
        values[0] = LSNGetDatum(backup_state->stoppoint);
        values[1] = CStringGetTextDatum(backup_label);
        values[2] = CStringGetTextDatum(tablespace_map->data);
+       values[3] = PointerGetDatum(pg_control_bytea);
 
        /* Deallocate backup-related variables */
        pfree(backup_label);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 999ea2ac801..ce717e8828c 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6767,8 +6767,8 @@
 { oid => '2739', descr => 'finish taking an online backup',
   proname => 'pg_backup_stop', provolatile => 'v', proparallel => 'r',
   prorettype => 'record', proargtypes => 'bool',
-  proallargtypes => '{bool,pg_lsn,text,text}', proargmodes => '{i,o,o,o}',
-  proargnames => '{wait_for_archive,lsn,labelfile,spcmapfile}',
+  proallargtypes => '{bool,pg_lsn,text,text,bytea}', proargmodes => 
'{i,o,o,o,o}',
+  proargnames => '{wait_for_archive,lsn,labelfile,spcmapfile,controlfile}',
   proargdefaults => '{true}',
   prosrc => 'pg_backup_stop',
   proacl => '{POSTGRES=X}' },
diff --git a/src/test/recovery/t/042_low_level_backup.pl 
b/src/test/recovery/t/042_low_level_backup.pl
index df4ae029fe6..34c6f4461af 100644
--- a/src/test/recovery/t/042_low_level_backup.pl
+++ b/src/test/recovery/t/042_low_level_backup.pl
@@ -13,6 +13,42 @@ use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
 
+# Decode hex to binary
+sub decode_hex
+{
+       my ($encoded) = @_;
+       my $decoded;
+
+       $encoded =~ s/^\s+|\s+$//g;
+
+       for (my $idx = 0; $idx < length($encoded); $idx += 2)
+       {
+               $decoded .= pack('C', hex(substr($encoded, $idx, 2)));
+       }
+
+       return $decoded;
+}
+
+# Get backup_label/pg_control from pg_stop_backup()
+sub stop_backup_result
+{
+       my ($psql) = @_;
+
+       my $encoded = $psql->query_safe(
+               "select encode(labelfile::bytea, 'hex') || ',' || " .
+               "       encode(controlfile, 'hex')" .
+               "  from pg_backup_stop()");
+
+       my @result;
+
+    foreach my $column (split(',', $encoded))
+       {
+               push(@result, decode_hex($column));
+       }
+
+       return @result;
+}
+
 # Start primary node with archiving.
 my $node_primary = PostgreSQL::Test::Cluster->new('primary');
 $node_primary->init(has_archiving => 1, allows_streaming => 1);
@@ -80,8 +116,7 @@ my $stop_segment_name = $node_primary->safe_psql('postgres',
        'SELECT pg_walfile_name(pg_current_wal_lsn())');
 
 # Stop backup and get backup_label, the last segment is archived.
-my $backup_label =
-  $psql->query_safe("select labelfile from pg_backup_stop()");
+(my $backup_label, my $pg_control) = stop_backup_result($psql);
 
 $psql->quit;
 
@@ -118,10 +153,36 @@ ok( $node_replica->log_contains(
 $node_replica->teardown_node;
 $node_replica->clean_node;
 
+# Save only pg_control into the backup to demonstrate that pg_control returned
+# from pg_stop_backup() will only perform recovery when backup_label is 
present.
+open(my $fh, ">", "$backup_dir/global/pg_control")
+  or die "could not open pg_control";
+binmode($fh);
+syswrite($fh, $pg_control);
+close($fh);
+
+$node_replica = PostgreSQL::Test::Cluster->new('replica_fail2');
+$node_replica->init_from_backup($node_primary, $backup_name,
+       has_restoring => 1);
+
+my $res = run_log(
+       [
+               'pg_ctl', '-D', $node_replica->data_dir, '-l',
+               $node_replica->logfile, 'start'
+       ]);
+ok(!$res, 'invalid recovery startup fails');
+
+my $logfile = slurp_file($node_replica->logfile());
+ok($logfile =~ qr/could not find backup_label required for recovery/,
+       'could not find backup_label required for recovery');
+
+$node_replica->teardown_node;
+$node_replica->clean_node;
+
 # Save backup_label into the backup directory and recover using the primary's
 # archive.  This time recovery will succeed and the canary table will be
 # present.
-open my $fh, ">>", "$backup_dir/backup_label"
+open $fh, ">>", "$backup_dir/backup_label"
   or die "could not open backup_label";
 # Binary mode is required for Windows, as the backup_label parsing is not
 # able to cope with CRLFs.
-- 
2.34.1

Reply via email to