public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/2] Common'ize file type checker for checksums
16+ messages / 6 participants
[nested] [flat]
* [PATCH 2/2] Common'ize file type checker for checksums
@ 2018-10-25 07:49 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2018-10-25 07:49 UTC (permalink / raw)
pg_verify_checksums.c and basebackup.c has the same notion of 'files
that have checksums'. This patch moves the core logic so that
src/common and the both files share the logic.
---
src/backend/replication/basebackup.c | 43 +++--
src/bin/pg_verify_checksums/Makefile | 3 +-
src/bin/pg_verify_checksums/pg_verify_checksums.c | 220 +---------------------
src/common/Makefile | 3 +-
src/common/file_checksums.c | 197 +++++++++++++++++++
src/include/common/file_checksums.h | 42 +++++
6 files changed, 273 insertions(+), 235 deletions(-)
create mode 100644 src/common/file_checksums.c
create mode 100644 src/include/common/file_checksums.h
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index b20f6c379c..4ebc969f3d 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -19,6 +19,7 @@
#include "access/xlog_internal.h" /* for pg_start/stop_backup */
#include "catalog/pg_type.h"
#include "common/file_perm.h"
+#include "common/file_checksums.h"
#include "lib/stringinfo.h"
#include "libpq/libpq.h"
#include "libpq/pqformat.h"
@@ -187,18 +188,6 @@ static const char *excludeFiles[] =
NULL
};
-/*
- * List of files excluded from checksum validation.
- */
-static const char *const noChecksumFiles[] = {
- "pg_control",
- "pg_filenode.map",
- "pg_internal.init",
- "PG_VERSION",
- NULL,
-};
-
-
/*
* Called when ERROR or FATAL happens in perform_base_backup() after
* we have started the backup - make sure we end it!
@@ -1321,22 +1310,36 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
static bool
is_checksummed_file(const char *fullpath, const char *filename)
{
- const char *const *f;
+ checksum_scan_context ctx;
/* Check that the file is in a tablespace */
if (strncmp(fullpath, "./global/", 9) == 0 ||
strncmp(fullpath, "./base/", 7) == 0 ||
strncmp(fullpath, "/", 1) == 0)
{
- /* Compare file against noChecksumFiles skiplist */
- for (f = noChecksumFiles; *f; f++)
- if (strcmp(*f, filename) == 0)
- return false;
+ /* check if the file has checksums */
+ switch (checksum_find_file_type(fullpath, NULL, &ctx))
+ {
+ case HEAP_TO_SCAN:
+ return true;
+ case STAT_FAILED:
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("failed to stat \"%s\": %m",
+ fullpath)));
- return true;
+ case ENTRY_TO_IGNORE:
+ case FILE_TO_SKIP:
+ case DIR_TO_SKIP:
+ case DIR_TO_SCAN:
+ break;
+ case FILE_UNKNOWN:
+ elog(DEBUG1, "checksum verification was skipped for unknown file: %s", fullpath);
+ break;
+ }
}
- else
- return false;
+
+ return false;
}
/*****
diff --git a/src/bin/pg_verify_checksums/Makefile b/src/bin/pg_verify_checksums/Makefile
index cfe4ab1b8b..3d0a9baf24 100644
--- a/src/bin/pg_verify_checksums/Makefile
+++ b/src/bin/pg_verify_checksums/Makefile
@@ -15,7 +15,8 @@ subdir = src/bin/pg_verify_checksums
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
-OBJS= pg_verify_checksums.o $(WIN32RES)
+OBJS= pg_verify_checksums.o $(top_builddir)/src/common/file_checksums.o \
+ $(WIN32RES)
all: pg_verify_checksums
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 4b527913c1..dc2143ea65 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -15,7 +15,7 @@
#include "catalog/pg_control.h"
#include "common/controldata_utils.h"
-#include "common/relpath.h"
+#include "common/file_checksums.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h"
@@ -36,46 +36,6 @@ static bool verbose = false;
static const char *progname;
-/* struct for checksum verification paremter*/
-typedef struct
-{
- union
- {
- struct
- {
- BlockNumber segmentno;
- } heap_param;
- } params;
-} checksum_scan_context;
-
-/* enum for return value of find_file_type */
-typedef enum
-{
- ENTRY_TO_IGNORE,
- DIR_TO_SCAN,
- HEAP_TO_SCAN,
- FILE_TO_SKIP,
- DIR_TO_SKIP,
- FILE_UNKNOWN
-} checksum_file_types;
-
-/* black (explisit exclusion) list for checksum verification */
-static const char *const checksum_known_to_skip[] = {
- "pg_control",
- "pg_internal.init",
- "pg_filenode.map",
- "PG_VERSION",
- "config_exec_params",
- "config_exec_params.new",
- "pgsql_tmp", /* this is a directory */
- NULL
-};
-
-static checksum_file_types find_file_type(const char *fn,
- const char *relfilenode,
- checksum_scan_context *ctx);
-
-
static void
usage(void)
{
@@ -93,71 +53,6 @@ usage(void)
printf(_("Report bugs to <[email protected]>.\n"));
}
-/*
- * isRelFileName
- *
- * Check if the given file name is authorized for checksum verification.
- */
-static bool
-isRelFileName(const char *fn)
-{
- int pos;
-
- /*----------
- * Only files including data checksums are authorized for verification.
- * This is guessed based on the file name by reverse-engineering
- * GetRelationPath() so make sure to update both code paths if any
- * updates are done. The following file name formats are allowed:
- * <digits>
- * <digits>.<segment>
- * <digits>_<forkname>
- * <digits>_<forkname>.<segment>
- *
- * Note that temporary files, beginning with 't', are also skipped.
- *
- *----------
- */
-
- /* A non-empty string of digits should follow */
- for (pos = 0; isdigit((unsigned char) fn[pos]); ++pos)
- ;
- /* leave if no digits */
- if (pos == 0)
- return false;
- /* good to go if only digits */
- if (fn[pos] == '\0')
- return true;
-
- /* Authorized fork files can be scanned */
- if (fn[pos] == '_')
- {
- int forkchar = forkname_chars(&fn[pos + 1], NULL);
-
- if (forkchar <= 0)
- return false;
-
- pos += forkchar + 1;
- }
-
- /* Check for an optional segment number */
- if (fn[pos] == '.')
- {
- int segchar;
-
- for (segchar = 1; isdigit((unsigned char) fn[pos + segchar]); ++segchar)
- ;
-
- if (segchar <= 1)
- return false;
- pos += segchar;
- }
-
- /* Now this should be the end */
- if (fn[pos] != '\0')
- return false;
- return true;
-}
-
static void
scan_heap_file(const char *fn, checksum_scan_context *ctx)
{
@@ -234,7 +129,7 @@ scan_directory(const char *basedir, const char *subdir)
checksum_scan_context ctx;
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
- switch (find_file_type(fn, only_relfilenode, &ctx))
+ switch (checksum_find_file_type(fn, only_relfilenode, &ctx))
{
case ENTRY_TO_IGNORE:
continue; /* ignore completely silently */
@@ -262,118 +157,16 @@ scan_directory(const char *basedir, const char *subdir)
case DIR_TO_SCAN:
scan_directory(path, de->d_name);
break;
+ case STAT_FAILED:
+ fprintf(stderr, _("%s: could not stat file \"%s\": %s\n"),
+ progname, fn, strerror(errno));
+ exit(1);
}
}
closedir(dir);
}
-/*
- * find_file_type: identify what to do on a file
- *
- * fn is a file path in full path or relative down from the current directory.
- * relfilenode is filter string of file. Only specified files of node number or
- * databaseid/filenodenum will be verified checksum.
- * ctx is the parameter needed for following checksum scan.
- */
-static checksum_file_types
-find_file_type(const char *fn, const char *relfilenode,
- checksum_scan_context *ctx)
-{
- struct stat st;
- char fnonly[MAXPGPATH];
- const char *fname;
- char *forkpath;
- char *segmentpath;
- const char *const *p;
- bool is_subdir = false;
-
- /* find file name the full path */
- fname = strrchr(fn, '/');
- if (fname)
- fname++;
- else
- fname = fn;
-
- if (strcmp(fname, ".") == 0 ||
- strcmp(fname, "..") == 0)
- return ENTRY_TO_IGNORE;
-
- if (lstat(fn, &st) < 0)
- {
- fprintf(stderr, _("%s: could not stat file \"%s\": %s\n"),
- progname, fn, strerror(errno));
- exit(1);
- }
-
-#ifndef WIN32
- if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
-#else
- if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
-#endif
- is_subdir = true;
-
- /* exluded by blacklist */
-
- for (p = checksum_known_to_skip ; *p ; p++)
- {
- if (strcmp(*p, fname) != 0)
- continue;
-
- if (!is_subdir)
- return FILE_TO_SKIP;
- else
- return DIR_TO_SKIP;
- }
-
- if (is_subdir)
- return DIR_TO_SCAN;
-
- /* we now know only of relfiles */
- if (isRelFileName(fname))
- {
- /* copy the path so that we can scribble on it */
- strlcpy(fnonly, fn, sizeof(fnonly));
- ctx->params.heap_param.segmentno = 0;
- segmentpath = strchr(fnonly, '.');
-
- /* make sure that the dot is in the last segment in the path */
- if (segmentpath != NULL && strchr(segmentpath, '/') == NULL)
- {
- *segmentpath++ = '\0';
- ctx->params.heap_param.segmentno = atoi(segmentpath);
-
- /* something's wrong, treat it as unknown file */
- if (ctx->params.heap_param.segmentno == 0)
- return FILE_UNKNOWN;
- }
-
- if (only_relfilenode)
- {
- char *p;
-
- /* find file suffix if any */
- forkpath = strrchr(fnonly, '_');
-
- /* the underscore must be in the last segment in the path */
- if (forkpath != NULL && strchr(forkpath, '/') == NULL)
- *forkpath++ = '\0';
-
- /* make a tail match with only_relfilenode */
- p = fnonly + strlen(fnonly) - strlen(relfilenode);
- if (fnonly > p || /* cannot match*/
- (fnonly < p && *(p-1) != '/') || /* avoid false match */
- strcmp(relfilenode, p) != 0)
- /* Relfilenode not to be included */
- return FILE_TO_SKIP;
- }
-
- return HEAP_TO_SCAN;
- }
-
- return FILE_UNKNOWN;
-}
-
int
main(int argc, char *argv[])
{
@@ -397,6 +190,7 @@ main(int argc, char *argv[])
if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
{
usage();
+
exit(0);
}
if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
diff --git a/src/common/Makefile b/src/common/Makefile
index ec8139f014..54b7c9f440 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -44,7 +44,8 @@ override CPPFLAGS += -DVAL_LIBS="\"$(LIBS)\""
override CPPFLAGS := -DFRONTEND $(CPPFLAGS)
LIBS += $(PTHREAD_LIBS)
-OBJS_COMMON = base64.o config_info.o controldata_utils.o exec.o file_perm.o \
+OBJS_COMMON = base64.o config_info.o controldata_utils.o exec.o \
+ file_checksums.o file_perm.o \
ip.o keywords.o link-canary.o md5.o pg_lzcompress.o \
pgfnames.o psprintf.o relpath.o \
rmtree.o saslprep.o scram-common.o string.o unicode_norm.o \
diff --git a/src/common/file_checksums.c b/src/common/file_checksums.c
new file mode 100644
index 0000000000..f83bb52c1d
--- /dev/null
+++ b/src/common/file_checksums.c
@@ -0,0 +1,197 @@
+/*-------------------------------------------------------------------------
+ * file_checksums.c
+ * checksumming files
+ *
+ * This implements Unicode normalization, per the documentation at
+ * http://www.unicode.org/reports/tr15/.
+ *
+ * Portions Copyright (c) 2018, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/common/file_checksums.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include <sys/stat.h>
+
+#include "c.h"
+#include "common/file_checksums.h"
+#include "common/relpath.h"
+
+/* black (explisit exclusion) list for checksum verification */
+static const char *const checksum_known_to_skip[] = {
+ "pg_control",
+ "pg_internal.init",
+ "pg_filenode.map",
+ "PG_VERSION",
+ "config_exec_params",
+ "config_exec_params.new",
+ "pgsql_tmp", /* directory */
+ NULL
+};
+
+/*
+ * isRelFileName
+ *
+ * Check if the given file name is authorized for checksum verification.
+ */
+static bool
+isRelFileName(const char *fn)
+{
+ int pos;
+
+ /*----------
+ * Only files including data checksums are authorized for verification.
+ * This is guessed based on the file name by reverse-engineering
+ * GetRelationPath() so make sure to update both code paths if any
+ * updates are done. The following file name formats are allowed:
+ * <digits>
+ * <digits>.<segment>
+ * <digits>_<forkname>
+ * <digits>_<forkname>.<segment>
+ *
+ * Note that temporary files, beginning with 't', are also skipped.
+ *
+ *----------
+ */
+
+ /* A non-empty string of digits should follow */
+ for (pos = 0; isdigit((unsigned char) fn[pos]); ++pos)
+ ;
+ /* leave if no digits */
+ if (pos == 0)
+ return false;
+ /* good to go if only digits */
+ if (fn[pos] == '\0')
+ return true;
+
+ /* Authorized fork files can be scanned */
+ if (fn[pos] == '_')
+ {
+ int forkchar = forkname_chars(&fn[pos + 1], NULL);
+
+ if (forkchar <= 0)
+ return false;
+
+ pos += forkchar + 1;
+ }
+
+ /* Check for an optional segment number */
+ if (fn[pos] == '.')
+ {
+ int segchar;
+
+ for (segchar = 1; isdigit((unsigned char) fn[pos + segchar]); ++segchar)
+ ;
+
+ if (segchar <= 1)
+ return false;
+ pos += segchar;
+ }
+
+ /* Now this should be the end */
+ if (fn[pos] != '\0')
+ return false;
+ return true;
+}
+
+/*
+ * checksum_find_file_type: identify a file from the viewpoint of checksum
+ *
+ * fn is file name with full path to check
+ * relfilenode is relfilenode in string to exclude files other than that.
+ * ctx is the context to scan checksum, which contains parameters for scanners.
+ */
+checksum_file_types
+checksum_find_file_type(const char *fn,
+ const char *relfilenode, checksum_scan_context *ctx)
+{
+ struct stat st;
+ char fnonly[MAXPGPATH];
+ char *fname;
+ char *forkpath;
+ char *segmentpath;
+ const char *const *p;
+ bool is_subdir = false;
+
+ fname = strrchr(fn, '/');
+
+ if (fname == NULL)
+ return ENTRY_TO_IGNORE;
+
+ fname++;
+
+ if (strcmp(fname, ".") == 0 ||
+ strcmp(fname, "..") == 0)
+ return ENTRY_TO_IGNORE;
+
+ if (lstat(fn, &st) < 0)
+ return STAT_FAILED;
+
+#ifndef WIN32
+ if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
+#else
+ if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
+#endif
+ is_subdir = true;
+
+ /* exluded by blacklist */
+
+ for (p = checksum_known_to_skip ; *p ; p++)
+ {
+ if (strcmp(*p, fname) != 0)
+ continue;
+
+ if (is_subdir)
+ return DIR_TO_SKIP;
+
+ return FILE_TO_SKIP;
+ }
+
+ if (is_subdir)
+ return DIR_TO_SCAN;
+
+ /* we now know only of relfiles */
+ if (isRelFileName(fname))
+ {
+ /* copy the path so that we can scribble on it */
+ strlcpy(fnonly, fn, sizeof(fnonly));
+ ctx->params.heap_param.segmentno = 0;
+ segmentpath = strchr(fnonly, '.');
+
+ /* make sure that the dot is in the last segment in the path */
+ if (segmentpath != NULL && strchr(segmentpath, '/') == NULL)
+ {
+ *segmentpath++ = '\0';
+ ctx->params.heap_param.segmentno = atoi(segmentpath);
+
+ /* something's wrong, treat it as unknown file */
+ if (ctx->params.heap_param.segmentno == 0)
+ return FILE_UNKNOWN;
+ }
+
+ if (relfilenode)
+ {
+ char *p;
+
+ /* find file suffix if any */
+ forkpath = strrchr(fnonly, '_');
+
+ /* the underscore must be in the last segment in the path */
+ if (forkpath != NULL && strchr(forkpath, '/') == NULL)
+ *forkpath++ = '\0';
+
+ /* make a tail match with only_relfilenode */
+ p = fnonly + strlen(fnonly) - strlen(relfilenode);
+ if (fnonly > p || /* cannot match*/
+ (fnonly < p && *(p-1) != '/') || /* avoid false match */
+ strcmp(relfilenode, p) != 0)
+ /* Relfilenode not to be included */
+ return FILE_TO_SKIP;
+ }
+
+ return HEAP_TO_SCAN;
+ }
+
+ return FILE_UNKNOWN;
+}
diff --git a/src/include/common/file_checksums.h b/src/include/common/file_checksums.h
new file mode 100644
index 0000000000..3ead25c97f
--- /dev/null
+++ b/src/include/common/file_checksums.h
@@ -0,0 +1,42 @@
+/*
+ * file_checksums.h
+ * checksumming files
+ *
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ *
+ * src/include/common/file_checksums.h
+ */
+#ifndef FILE_CHECKSUMS_H
+#define FILE_CHECKSUMS_H
+
+#include "storage/block.h"
+
+/* struct for checksum verification paremter*/
+typedef struct
+{
+ union
+ {
+ struct
+ {
+ BlockNumber segmentno;
+ } heap_param;
+ } params;
+} checksum_scan_context;
+
+/* enum for return value of find_file_type */
+typedef enum
+{
+ ENTRY_TO_IGNORE,
+ DIR_TO_SCAN,
+ HEAP_TO_SCAN,
+ FILE_TO_SKIP,
+ DIR_TO_SKIP,
+ FILE_UNKNOWN,
+ STAT_FAILED
+} checksum_file_types;
+
+checksum_file_types checksum_find_file_type(const char *fn,
+ const char *relfilenode,
+ checksum_scan_context *ctx);
+
+#endif /* FILE_CHECKSUMS_H */
--
2.16.3
----Next_Part(Thu_Oct_25_17_57_42_2018_474)----
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres
@ 2024-02-12 23:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw)
With the addition of the proposed streaming read API [1],
table_scan_bitmap_next_block() will no longer take a TBMIterateResult as
an input. Instead table AMs will be responsible for implementing a
callback for the streaming read API which specifies which blocks should
be prefetched and read.
Thus, it no longer makes sense to use the TBMIterateResult as a means of
communication between table_scan_bitmap_next_tuple() and
table_scan_bitmap_next_block().
Note that this parameter was unused by heap AM's implementation of
table_scan_bitmap_next_tuple().
[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2...
---
src/backend/access/heap/heapam_handler.c | 1 -
src/backend/executor/nodeBitmapHeapscan.c | 2 +-
src/include/access/tableam.h | 12 +-----------
3 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 849cac3947..cf4387f443 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
static bool
heapam_scan_bitmap_next_tuple(TableScanDesc scan,
- TBMIterateResult *tbmres,
TupleTableSlot *slot)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 49938c9ed4..282dcb9791 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
/*
* Attempt to fetch tuple from AM.
*/
- if (!table_scan_bitmap_next_tuple(scan, tbmres, slot))
+ if (!table_scan_bitmap_next_tuple(scan, slot))
{
/* nothing more to look at on this page */
node->tbmres = tbmres = NULL;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b9ba4f9fb3..bcf1497f67 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -795,10 +795,7 @@ typedef struct TableAmRoutine
*
* This will typically read and pin the target block, and do the necessary
* work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
- * make sense to perform tuple visibility checks at this time). For some
- * AMs it will make more sense to do all the work referencing `tbmres`
- * contents here, for others it might be better to defer more work to
- * scan_bitmap_next_tuple.
+ * make sense to perform tuple visibility checks at this time).
*
* If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples
* on the page have to be returned, otherwise the tuples at offsets in
@@ -829,15 +826,10 @@ typedef struct TableAmRoutine
* Fetch the next tuple of a bitmap table scan into `slot` and return true
* if a visible tuple was found, false otherwise.
*
- * For some AMs it will make more sense to do all the work referencing
- * `tbmres` contents in scan_bitmap_next_block, for others it might be
- * better to defer more work to this callback.
- *
* Optional callback, but either both scan_bitmap_next_block and
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_tuple) (TableScanDesc scan,
- struct TBMIterateResult *tbmres,
TupleTableSlot *slot);
/*
@@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan,
*/
static inline bool
table_scan_bitmap_next_tuple(TableScanDesc scan,
- struct TBMIterateResult *tbmres,
TupleTableSlot *slot)
{
/*
@@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
- tbmres,
slot);
}
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0009-Make-table_scan_bitmap_next_block-async-friendly.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
@ 2026-02-20 03:10 David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: David Steele @ 2026-02-20 03:10 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On 8/7/25 05:30, David Steele wrote:
> On 1/24/25 13:43, David Steele wrote:
>>
>> Rebased and improved a comment and an error.
> Rebased to fix breakage caused by the split of func.sgml in 4e23c9e.
Rebased to implement simplification added by "Simplify creation of
built-in functions with default arguments" (759b03b2).
Regards,
-David
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-02-20 05:47 ` David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: David Steele @ 2026-02-20 05:47 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On 2/20/26 10:10, David Steele wrote:
> On 8/7/25 05:30, David Steele wrote:
>> On 1/24/25 13:43, David Steele wrote:
>>>
>>> Rebased and improved a comment and an error.
>> Rebased to fix breakage caused by the split of func.sgml in 4e23c9e.
>
>
> Rebased to implement simplification added by "Simplify creation of
> built-in functions with default arguments" (759b03b2).
With the patches this time!
Regards,
-David
From f4043472a7acb4c74566ac17026d0e4b6245aa58 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 20 Feb 2026 02:59:52 +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 | 23 +++++++++++++++++++++++
src/backend/access/transam/xlogrecovery.c | 10 +++++++++-
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, 80 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index 294f45e82a3..2e78d28221c 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3646,6 +3646,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 13cce9b49f1..babb76b4710 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9586,6 +9586,29 @@ 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 + sizeof(ControlFileData), 0,
+ PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
+
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+ 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 c0c2744d45b..18a0db611e6 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -710,7 +710,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
@@ -995,6 +1002,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
{
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 2d74c648335..f54c8793090 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"
@@ -331,9 +332,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");
@@ -356,14 +357,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 85dc43d4cdb..5eddebe27f2 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 d0aafd7e7a6..d5b49b2a26b 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 fdfb572467b..84ec5aabdfd 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -311,6 +311,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 7503db1af51..26618162a7b 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 dac40992cbc..3c368433615 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12437,9 +12437,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 e654c44aac81c2aa5b70b03a62067b0caedcae1c Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 20 Feb 2026 02:59: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 2efe4105efb..e7e7c0b18e5 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -113,9 +113,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.
@@ -123,12 +125,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 */
@@ -150,9 +154,17 @@ pg_backup_stop(PG_FUNCTION_ARGS)
/* Build the contents of backup_label */
backup_label = build_backup_content(backup_state, false);
+ /* 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);
+
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 3c368433615..c89967613a5 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6741,8 +6741,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' },
{ oid => '3436', descr => 'promote standby server',
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
Attachments:
[text/plain] pgcontrol-flag-v7-01-basebackup.patch (11.5K, ../../[email protected]/2-pgcontrol-flag-v7-01-basebackup.patch)
download | inline diff:
From f4043472a7acb4c74566ac17026d0e4b6245aa58 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 20 Feb 2026 02:59:52 +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 | 23 +++++++++++++++++++++++
src/backend/access/transam/xlogrecovery.c | 10 +++++++++-
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, 80 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index 294f45e82a3..2e78d28221c 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3646,6 +3646,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 13cce9b49f1..babb76b4710 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9586,6 +9586,29 @@ 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 + sizeof(ControlFileData), 0,
+ PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
+
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+ 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 c0c2744d45b..18a0db611e6 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -710,7 +710,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
@@ -995,6 +1002,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
{
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 2d74c648335..f54c8793090 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"
@@ -331,9 +332,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");
@@ -356,14 +357,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 85dc43d4cdb..5eddebe27f2 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 d0aafd7e7a6..d5b49b2a26b 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 fdfb572467b..84ec5aabdfd 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -311,6 +311,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 7503db1af51..26618162a7b 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 dac40992cbc..3c368433615 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12437,9 +12437,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
[text/plain] pgcontrol-flag-v7-02-sql.patch (9.7K, ../../[email protected]/3-pgcontrol-flag-v7-02-sql.patch)
download | inline diff:
From e654c44aac81c2aa5b70b03a62067b0caedcae1c Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 20 Feb 2026 02:59: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 2efe4105efb..e7e7c0b18e5 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -113,9 +113,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.
@@ -123,12 +125,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 */
@@ -150,9 +154,17 @@ pg_backup_stop(PG_FUNCTION_ARGS)
/* Build the contents of backup_label */
backup_label = build_backup_content(backup_state, false);
+ /* 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);
+
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 3c368433615..c89967613a5 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6741,8 +6741,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' },
{ oid => '3436', descr => 'promote standby server',
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-03-06 01:27 ` David Steele <[email protected]>
2026-03-17 05:51 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
0 siblings, 2 replies; 16+ messages in thread
From: David Steele @ 2026-03-06 01:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On 2/20/26 12:47, David Steele wrote:
> On 2/20/26 10:10, David Steele wrote:
>> On 8/7/25 05:30, David Steele wrote:
>>> On 1/24/25 13:43, David Steele wrote:
>>>>
>>>> Rebased and improved a comment and an error.
>>> Rebased to fix breakage caused by the split of func.sgml in 4e23c9e.
>>
>>
>> Rebased to implement simplification added by "Simplify creation of
>> built-in functions with default arguments" (759b03b2).
Rebased on "Simplify creation of built-in functions with non-default
ACLs." (f95d73ed).
Regards,
-David
From a38d5e68b648e1d03832c3b917a60a2a5ed4c61b Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 6 Mar 2026 01:10:14 +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 | 23 +++++++++++++++++++++++
src/backend/access/transam/xlogrecovery.c | 10 +++++++++-
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, 80 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index 294f45e82a3..2e78d28221c 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3646,6 +3646,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 354ac645bdc..66f0d7e091d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9563,6 +9563,29 @@ 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 + sizeof(ControlFileData), 0,
+ PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
+
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+ 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 fbddd7e522c..71759ae67b3 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -646,7 +646,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
@@ -931,6 +938,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
{
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 2d74c648335..f54c8793090 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"
@@ -331,9 +332,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");
@@ -356,14 +357,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 fdfb572467b..84ec5aabdfd 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -311,6 +311,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 7503db1af51..26618162a7b 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 4950bff2804..ece9b4f08c9 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12494,9 +12494,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 1de5c13947884e33f744177d7f21147826a180e1 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 6 Mar 2026 01:10:14 +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 2efe4105efb..e7e7c0b18e5 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -113,9 +113,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.
@@ -123,12 +125,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 */
@@ -150,9 +154,17 @@ pg_backup_stop(PG_FUNCTION_ARGS)
/* Build the contents of backup_label */
backup_label = build_backup_content(backup_state, false);
+ /* 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);
+
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 ece9b4f08c9..9a85bc987df 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6757,8 +6757,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
Attachments:
[text/plain] pgcontrol-flag-v8-01-basebackup.patch (11.5K, ../../[email protected]/2-pgcontrol-flag-v8-01-basebackup.patch)
download | inline diff:
From a38d5e68b648e1d03832c3b917a60a2a5ed4c61b Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 6 Mar 2026 01:10:14 +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 | 23 +++++++++++++++++++++++
src/backend/access/transam/xlogrecovery.c | 10 +++++++++-
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, 80 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index 294f45e82a3..2e78d28221c 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3646,6 +3646,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 354ac645bdc..66f0d7e091d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9563,6 +9563,29 @@ 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 + sizeof(ControlFileData), 0,
+ PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
+
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+ 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 fbddd7e522c..71759ae67b3 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -646,7 +646,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
@@ -931,6 +938,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
{
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 2d74c648335..f54c8793090 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"
@@ -331,9 +332,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");
@@ -356,14 +357,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 fdfb572467b..84ec5aabdfd 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -311,6 +311,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 7503db1af51..26618162a7b 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 4950bff2804..ece9b4f08c9 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12494,9 +12494,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
[text/plain] pgcontrol-flag-v8-02-sql.patch (9.7K, ../../[email protected]/3-pgcontrol-flag-v8-02-sql.patch)
download | inline diff:
From 1de5c13947884e33f744177d7f21147826a180e1 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Fri, 6 Mar 2026 01:10:14 +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 2efe4105efb..e7e7c0b18e5 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -113,9 +113,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.
@@ -123,12 +125,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 */
@@ -150,9 +154,17 @@ pg_backup_stop(PG_FUNCTION_ARGS)
/* Build the contents of backup_label */
backup_label = build_backup_content(backup_state, false);
+ /* 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);
+
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 ece9b4f08c9..9a85bc987df 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6757,8 +6757,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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-03-17 05:51 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 16+ messages in thread
From: Michael Paquier @ 2026-03-17 05:51 UTC (permalink / raw)
To: Haibo Yan <[email protected]>; +Cc: David Steele <[email protected]>; pgsql-hackers
On Mon, Mar 16, 2026 at 10:16:58PM -0700, Haibo Yan wrote:
> I have not read the code yet, so this may already be answered there,
> but I had a question about the proposal itself. This patch protects
> against a missing backup_label, but what about a wrong one? If a
> user restores a backup_label file from a different backup, the
> existence check alone would not detect that. Do we need some
> consistency check between the returned pg_control copy and the
> backup_label contents, or is the intended scope here limited to the
> “missing file” case only?
Please note that we use bottom-posting on the lists, as of:
https://en.wikipedia.org/wiki/Posting_style#Bottom-posting
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-03-17 07:05 ` David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: David Steele @ 2026-03-17 07:05 UTC (permalink / raw)
To: Haibo Yan <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
On 3/17/26 12:16, Haibo Yan wrote:
> I have not read the code yet, so this may already be answered there, but
> I had a question about the proposal itself. This patch protects against
> a missing backup_label, but what about a wrong one? If a user restores a
> backup_label file from a different backup, the existence check alone
> would not detect that. Do we need some consistency check between the
> returned pg_control copy and the backup_label contents, or is the
> intended scope here limited to the “missing file” case only?
Thank you for having a look!
The goal here is only to check for a missing backup_label. The general
problem is that PostgreSQL suggests that removing backup_label might be
a good idea so the user does it:
If you are not restoring from a backup, try removing the file
\"%s/backup_label\"
The user *could* copy a backup_label from another backup and there are
ways we could detect that but I feel that should be material for a
separate patch.
Regards,
-David
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-03-17 18:50 ` Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Haibo Yan @ 2026-03-17 18:50 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
Hi David,
Thank you for the clarification. I have now read the code, and overall it looks good to me. I only had one very small comment.
You currently have:
```
memset(controlFile + sizeof(ControlFileData), 0,
PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
memcpy(controlFile, ControlFile, sizeof(ControlFileData));
```
This is correct, since only the trailing bytes need to be zeroed before the copy.
I was just wondering whether the following might be slightly clearer:
```
memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
memcpy(controlFile, ControlFile, sizeof(ControlFileData));
```
I do not think this is a real issue, though.
Thanks
Haibo
> On Mar 17, 2026, at 12:05 AM, David Steele <[email protected]> wrote:
>
> On 3/17/26 12:16, Haibo Yan wrote:
>
>> I have not read the code yet, so this may already be answered there, but I had a question about the proposal itself. This patch protects against a missing backup_label, but what about a wrong one? If a user restores a backup_label file from a different backup, the existence check alone would not detect that. Do we need some consistency check between the returned pg_control copy and the backup_label contents, or is the intended scope here limited to the “missing file” case only?
>
> Thank you for having a look!
>
> The goal here is only to check for a missing backup_label. The general problem is that PostgreSQL suggests that removing backup_label might be a good idea so the user does it:
>
> If you are not restoring from a backup, try removing the file \"%s/backup_label\"
>
> The user *could* copy a backup_label from another backup and there are ways we could detect that but I feel that should be material for a separate patch.
>
> Regards,
> -David
>
>
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
@ 2026-03-18 01:43 ` Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2026-03-18 01:43 UTC (permalink / raw)
To: Haibo Yan <[email protected]>; +Cc: David Steele <[email protected]>; pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
On Tue, Mar 17, 2026 at 11:50:29AM -0700, Haibo Yan wrote:
> Thank you for the clarification. I have now read the code, and
> overall it looks good to me. I only had one very small comment.
(Bottom-posting note from above, please be careful.)
> I was just wondering whether the following might be slightly clearer:
> ```
> memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
> memcpy(controlFile, ControlFile, sizeof(ControlFileData));
> ```
>
> I do not think this is a real issue, though.
{
ControlFile->backupStartPoint = checkPoint.redo;
ControlFile->backupEndRequired = backupEndRequired;
+ ControlFile->backupLabelRequired = false;
It sounds like it is going to be important to document the reason why
the flag is reset here (aka we don't need the backup_label file
anymore).
+backup_control_file(uint8_t *controlFile)
+{
+ ControlFileData *controlData = ((ControlFileData *)controlFile);
+
+ memset(controlFile + sizeof(ControlFileData), 0,
+ PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
+
+ LWLockAcquire(ControlFileLock, LW_SHARED);
+ memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+ LWLockRelease(ControlFileLock);
+
+ controlData->backupLabelRequired = true;
+
+ INIT_CRC32C(controlData->crc);
+ COMP_CRC32C(controlData->crc, controlFile, offsetof(ControlFileData, crc));
+ FIN_CRC32C(controlData->crc);
I was wondering if we should have an assertion at least to cross-check
that the contents we store in shared memory never go out-of-sync with
the on-disk contents, in the shape of a USE_ASSERT_CHECKING block that
calls get_controlfile() and memcmp()'s the contents between shmem and
the on-disk file, while the LWLock is taken. We ship the control file
last on purpose, one reason being backups taken from standbys, so that
may be sensible to do.
Another property of the new control file flag that is implied in the
implementation but not documented is that we should never check for
backupLabelRequired when a backup_label is gone. Actually, the flag
is reset in InitWalRecovery() in the initial steps of recovery, and
the backup_label file is removed much later in StartupXLOG() just
*after* UpdateControlFile() to minimize the window where the contents
of the control file and the backup_label file is removed are
out-of-sync. This window means that if we crash between the
completion of UpdateControlFile() and the durable_rename() we could
have a flag reset with a backup_label still around. On restart,
recovery would fail, requiring a manual modification of the control
file, at least. It sounds to me that this implementation detail
should be documented clearly.
Finally, here is a general opinion. I like this patch, and it is
basically risk-free for base backups taken with the replication
protocol as we update the control file with the new flag set
on-the-fly.
Now, I am worried about backups that use pg_stop_backup(). Changing
backup APIs has always been a very sensitive area, and this is going
to require operators to update backup tools so as the control file
received as a result of pg_stop_backup() is copied, at the cost of
getting a failure if they don't do so. I will *not* proceed with this
change without a clear approval from some more committers or senior
hackers that they like this change (approach previously suggested by
Andres, actually, for what I can see). I am adding in CC a few
committers who have commented on this set of proposals and who have
touched the recovery code in the last few years, for awareness.
The timing is what it is, and we are at the end of a release cycle.
Let's see if we can reach a consensus of some kind.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
@ 2026-03-18 04:53 ` Michael Paquier <[email protected]>
2026-03-18 08:26 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2026-03-18 04:53 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Haibo Yan <[email protected]>; pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
On Wed, Mar 18, 2026 at 04:05:24AM +0000, David Steele wrote:
> On 3/18/26 08:43, Michael Paquier wrote:
>> I was wondering if we should have an assertion at least to cross-check
>> that the contents we store in shared memory never go out-of-sync with
>> the on-disk contents, in the shape of a USE_ASSERT_CHECKING block that
>> calls get_controlfile() and memcmp()'s the contents between shmem and
>> the on-disk file, while the LWLock is taken. We ship the control file
>> last on purpose, one reason being backups taken from standbys, so that
>> may be sensible to do.
>
> As far as I can see this should always be true -- I audited all the
>
> LWLockAcquire(ControlFileLock, LW_EXCLUSIVE)
>
> sections and the file is always saved once if is updated. Let me see if I
> can add this check without too much pain, e.g. an additional parameter.
This matches with my reads of the code. The attached check, that can
be applied on top of your patches, passes under check-world.
>> Another property of the new control file flag that is implied in the
>> implementation but not documented is that we should never check for
>> backupLabelRequired when a backup_label is gone.
>
> I'm not sure what you mean here? That's exactly when we do want to check as
> below:
Sorry for the confusion, I meant that "we should never check for
backupLabelRequired when we have a backup_label".
> Using the pg_control copy from pg_backup_stop() is entirely optional and
> nothing is broken if vendors decide not to use it. This can be demonstrated
> by applying the 01 patch without 02. In that case the tests in
> 042_low_level_backup continue to run. You can also apply 01 and 02 and
> revert the test changes in 042_low_level_backup and that works, too.
FWIW, after a second look I am actually wondering if 0002 is safe at
all. The contents of the control file are fetched after we are done
with do_pg_backup_stop(), and there could be a bunch of activity that
happens between the end of do_pg_backup_stop() and the
backup_control_file() call, where the control file could be updated
more and interfere with the recovery startup for some of its fields?
GUC parameter updates that may touch the control file are one thing
popping into mind.
--
Michael
From de5b9b00867e39e73dc99a1ee61a814cead1ab7d Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Wed, 18 Mar 2026 13:44:01 +0900
Subject: [PATCH] Add consistency check for shmem and on-disk control file
---
src/backend/access/transam/xlog.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c8ea1ea49d24..0f8516fcb563 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9606,6 +9606,17 @@ backup_control_file(uint8_t *controlFile)
LWLockAcquire(ControlFileLock, LW_SHARED);
memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+
+#ifdef USE_ASSERT_CHECKING
+ {
+ 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;
--
2.53.0
Attachments:
[text/plain] 0001-Add-consistency-check-for-shmem-and-on-disk-control-.patch (1007B, ../../[email protected]/2-0001-Add-consistency-check-for-shmem-and-on-disk-control-.patch)
download | inline diff:
From de5b9b00867e39e73dc99a1ee61a814cead1ab7d Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Wed, 18 Mar 2026 13:44:01 +0900
Subject: [PATCH] Add consistency check for shmem and on-disk control file
---
src/backend/access/transam/xlog.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c8ea1ea49d24..0f8516fcb563 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9606,6 +9606,17 @@ backup_control_file(uint8_t *controlFile)
LWLockAcquire(ControlFileLock, LW_SHARED);
memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+
+#ifdef USE_ASSERT_CHECKING
+ {
+ 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;
--
2.53.0
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
@ 2026-03-18 08:26 ` Michael Paquier <[email protected]>
2026-03-18 12:26 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2026-03-18 08:26 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Haibo Yan <[email protected]>; pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
On Wed, Mar 18, 2026 at 07:35:47AM +0000, David Steele wrote:
> You are correct -- the copy of pg_control needs to happen before
> do_pg_backup_stop(). An older version of this patch saved pg_control in
> backup_state which made the prior location safe. However, I missed moving
> this code when I moved pg_control out of backup_state. Code review to the
> rescue.
Right. I am wondering also if the final result would not be better
without 0002, actually, focusing only on the "simpler" base backup
case through the replication protocol, and you are making a good case
in mentioning it as not absolutely mandatory for base backups that are
taken through the SQL functions. One could always tweak the flag
manually in the control file based on the contents taken from the data
folder. That's more hairy than writing the entire file, for sure,
still possible.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 08:26 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
@ 2026-03-18 12:26 ` David Steele <[email protected]>
2026-03-18 21:00 ` Re: Return pg_control from pg_backup_stop(). Corey Huinker <[email protected]>
2026-04-13 14:55 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
0 siblings, 2 replies; 16+ messages in thread
From: David Steele @ 2026-03-18 12:26 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Haibo Yan <[email protected]>; pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
On 3/18/26 15:26, Michael Paquier wrote:
> On Wed, Mar 18, 2026 at 07:35:47AM +0000, David Steele wrote:
>> You are correct -- the copy of pg_control needs to happen before
>> do_pg_backup_stop(). An older version of this patch saved pg_control in
>> backup_state which made the prior location safe. However, I missed moving
>> this code when I moved pg_control out of backup_state. Code review to the
>> rescue.
>
> Right. I am wondering also if the final result would not be better
> without 0002, actually, focusing only on the "simpler" base backup
> case through the replication protocol, and you are making a good case
> in mentioning it as not absolutely mandatory for base backups that are
> taken through the SQL functions. One could always tweak the flag
> manually in the control file based on the contents taken from the data
> folder. That's more hairy than writing the entire file, for sure,
> still possible.
Getting even 01 into PG19 would be a great outcome. This would solve the
problem of torn pg_control and deleted backup labels for any backups
made with pg_basebackup and that's going to cover a *lot* of cases.
Established third-party backup solutions that are not based on
pg_basebackup are generally able to manipulate pg_control so that's not
as much of a concern, perhaps. It does raise the barrier of entry for
new backup software if they need to learn to read and validate
pg_control to avoid a torn copy and set the flag. Patch 02 solves that
problem in a general way so I still think it adds value for the
ecosystem -- but we could always discuss that in the PG20 cycle.
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.
Regards,
-David
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 08:26 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 12:26 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-03-18 21:00 ` Corey Huinker <[email protected]>
2026-03-19 04:20 ` Re: Return pg_control from pg_backup_stop(). Corey Huinker <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: Corey Huinker @ 2026-03-18 21:00 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Michael Paquier <[email protected]>; Haibo Yan <[email protected]>; pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
>
> 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. 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()). 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.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 08:26 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 12:26 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-18 21:00 ` Re: Return pg_control from pg_backup_stop(). Corey Huinker <[email protected]>
@ 2026-03-19 04:20 ` Corey Huinker <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Corey Huinker @ 2026-03-19 04:20 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Michael Paquier <[email protected]>; Haibo Yan <[email protected]>; pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
On Wed, Mar 18, 2026 at 10:56 PM David Steele <[email protected]> wrote:
> 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.
That's always been my assumption. Not worth the churn.
> 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 limited my git log -p to elog.h, and it seems it started with
e3a87b4991cc back in 2020. The only reason I knew about it was that I used
to do backports from v13 to unsupported versions, and the new style would
cause the build to fail on an otherwise clean cherry pick.
> It seems to me you've still done a review. Confirming what the other
> reviewers found is good info to have.
>
Of a sort, yes, but our review doesn't touch the "is this a good idea"
question, which has been by far the thing most in need of reviewing across
the long discussion threads.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 08:26 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 12:26 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-04-13 14:55 ` David Steele <[email protected]>
2026-06-30 05:15 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: David Steele @ 2026-04-13 14:55 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Haibo Yan <[email protected]>; pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
On 3/18/26 19:26, David Steele wrote:
> On 3/18/26 15:26, Michael Paquier wrote:
>> On Wed, Mar 18, 2026 at 07:35:47AM +0000, David Steele wrote:
>>> You are correct -- the copy of pg_control needs to happen before
>>> do_pg_backup_stop(). An older version of this patch saved pg_control in
>>> backup_state which made the prior location safe. However, I missed
>>> moving
>>> this code when I moved pg_control out of backup_state. Code review to
>>> the
>>> rescue.
>>
>> Right. I am wondering also if the final result would not be better
>> without 0002, actually, focusing only on the "simpler" base backup
>> case through the replication protocol, and you are making a good case
>> in mentioning it as not absolutely mandatory for base backups that are
>> taken through the SQL functions. One could always tweak the flag
>> manually in the control file based on the contents taken from the data
>> folder. That's more hairy than writing the entire file, for sure,
>> still possible.
>
> Getting even 01 into PG19 would be a great outcome. This would solve the
> problem of torn pg_control and deleted backup labels for any backups
> made with pg_basebackup and that's going to cover a *lot* of cases.
>
> Established third-party backup solutions that are not based on
> pg_basebackup are generally able to manipulate pg_control so that's not
> as much of a concern, perhaps. It does raise the barrier of entry for
> new backup software if they need to learn to read and validate
> pg_control to avoid a torn copy and set the flag. Patch 02 solves that
> problem in a general way so I still think it adds value for the
> ecosystem -- but we could always discuss that in the PG20 cycle.
>
> 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.
I have withdrawn this patch. If anybody wants to pick it up in the
future I'll be happy to rebase it but I think two years is long enough
to maintain a patch that is not getting traction.
We are left with the issue that pg_basebackup backups may contain a torn
copy of pg_control. At the least this should be documented.
It would also be a good idea to document that utilizing the low-level
backup interface requires validating the checksum in pg_control to avoid
a torn copy. This is non-trivial but certainly doable.
Regards,
-David
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Return pg_control from pg_backup_stop().
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 08:26 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 12:26 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-04-13 14:55 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
@ 2026-06-30 05:15 ` David Steele <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: David Steele @ 2026-06-30 05:15 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Fujii Masao <[email protected]>
On 4/13/26 21:55, David Steele wrote:
>
> I have withdrawn this patch. If anybody wants to pick it up in the
> future I'll be happy to rebase it but I think two years is long enough
> to maintain a patch that is not getting traction.
I would like to revive this but focus on the first patch for now and
drop the second patch from consideration.
The patch now implements only the new flag for pgcontrol and wires this
logic into basebackup. It has the additional benefit of guaranteeing
that the base backup contains a non-torn version of pgcontrol.
I know everyone was really busy in March but now that things are a bit
calmer I'd like to revisit.
Heikki, Robert, Andres, Fujii -- any objections or comments? I believe
Michael is on board with the feature (this part, at least) but he very
sensibly would like to have some consensus.
Thanks,
-David
From 6859828e85c7d54ab71b4dc3e01c45a47b89c469 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Tue, 30 Jun 2026 04:42:47 +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 | 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/t/002_archiving.pl | 20 +++++++++++
12 files changed, 110 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index 69ef3857cfa..58d7e9adb19 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3649,6 +3649,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 a81912b7441..7bfd193c59b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10074,6 +10074,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 c0ae4d3f63f..5668d13d788 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -648,7 +648,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
@@ -928,11 +935,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 9c79dadaacc..4730ec2ac65 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 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 fe5fc5ec133..cfe19f4090a 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -303,6 +303,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 f8d25afed9d..848ed8bcdb0 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 4dd98624204..182b2fe8f7c 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -326,6 +326,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 80b3a730e03..2ad1254adf1 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 1a985becde3..dd20fe1402a 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12354,9 +12354,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
Attachments:
[text/plain] pgcontrol-basebackup-flag-v11.patch (12.8K, ../../[email protected]/2-pgcontrol-basebackup-flag-v11.patch)
download | inline diff:
From 6859828e85c7d54ab71b4dc3e01c45a47b89c469 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Tue, 30 Jun 2026 04:42:47 +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 | 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/t/002_archiving.pl | 20 +++++++++++
12 files changed, 110 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
index 69ef3857cfa..58d7e9adb19 100644
--- a/doc/src/sgml/func/func-info.sgml
+++ b/doc/src/sgml/func/func-info.sgml
@@ -3649,6 +3649,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 a81912b7441..7bfd193c59b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10074,6 +10074,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 c0ae4d3f63f..5668d13d788 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -648,7 +648,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
@@ -928,11 +935,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 9c79dadaacc..4730ec2ac65 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 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 fe5fc5ec133..cfe19f4090a 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -303,6 +303,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 f8d25afed9d..848ed8bcdb0 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 4dd98624204..182b2fe8f7c 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -326,6 +326,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 80b3a730e03..2ad1254adf1 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 1a985becde3..dd20fe1402a 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12354,9 +12354,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
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2026-06-30 05:15 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 07:49 [PATCH 2/2] Common'ize file type checker for checksums Kyotaro Horiguchi <[email protected]>
2024-02-12 23:13 [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]>
2026-02-20 03:10 Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-02-20 05:47 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-06 01:27 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 05:51 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-17 07:05 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-17 18:50 ` Re: Return pg_control from pg_backup_stop(). Haibo Yan <[email protected]>
2026-03-18 01:43 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 04:53 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 08:26 ` Re: Return pg_control from pg_backup_stop(). Michael Paquier <[email protected]>
2026-03-18 12:26 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-03-18 21:00 ` Re: Return pg_control from pg_backup_stop(). Corey Huinker <[email protected]>
2026-03-19 04:20 ` Re: Return pg_control from pg_backup_stop(). Corey Huinker <[email protected]>
2026-04-13 14:55 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
2026-06-30 05:15 ` Re: Return pg_control from pg_backup_stop(). David Steele <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox