Add the system identifier to backup manifests.

Before this patch, if you took a full backup on server A and then
tried to use the backup manifest to take an incremental backup on
server B, it wouldn't know that the manifest was from a different
server and so the incremental backup operation could potentially
complete without error. When you later tried to run pg_combinebackup,
you'd find out that your incremental backup was and always had been
invalid. That's poor timing, because nobody likes finding out about
backup problems only at restore time.

With this patch, you'll get an error when trying to take the (invalid)
incremental backup, which seems a lot nicer.

Amul Sul, revised by me. Review by Michael Paquier.

Discussion: http://postgr.es/m/CA+TgmoYLZzbSAMM3cAjV4Y+iCRZn-bR9H2+Mdz7NdaJFU1Zb5w@mail.gmail.com

Author: robertmhaas

doc/src/sgml/backup-manifest.sgml

@@ -37,7 +37,22 @@
     <term><literal>PostgreSQL-Backup-Manifest-Version</literal></term>
     <listitem>
      <para>
-      The associated value is always the integer 1.
+      The associated value is an integer. Beginning in
+      <productname>PostgreSQL</productname> <literal>17</literal>,
+      it is <literal>2</literal>; in older versions, it is <literal>1</literal>.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry>
+    <term><literal>System-Identifier</literal></term>
+    <listitem>
+     <para>
+      The database system identifier of the
+      <productname>PostgreSQL</productname> instance where the backup was
+      taken.  This field is present only when
+      <literal>PostgreSQL-Backup-Manifest-Version</literal> is
+      <literal>2</literal>.
      </para>
     </listitem>
    </varlistentry>

doc/src/sgml/ref/pg_verifybackup.sgml

@@ -53,9 +53,10 @@ PostgreSQL documentation
    Backup verification proceeds in four stages. First,
    <literal>pg_verifybackup</literal> reads the
    <literal>backup_manifest</literal> file. If that file
-   does not exist, cannot be read, is malformed, or fails verification
-   against its own internal checksum, <literal>pg_verifybackup</literal>
-   will terminate with a fatal error.
+   does not exist, cannot be read, is malformed, fails to match the system
+   identifier with <filename>pg_control</filename> of the backup directory or
+   fails verification against its own internal checksum,
+   <literal>pg_verifybackup</literal> will terminate with a fatal error.
   </para>
 
   <para>

src/backend/backup/backup_manifest.c

@@ -13,6 +13,7 @@
 #include "postgres.h"
 
 #include "access/timeline.h"
+#include "access/xlog.h"
 #include "backup/backup_manifest.h"
 #include "backup/basebackup_sink.h"
 #include "mb/pg_wchar.h"
@@ -77,8 +78,10 @@ InitializeBackupManifest(backup_manifest_info *manifest,
 
 	if (want_manifest != MANIFEST_OPTION_NO)
 		AppendToManifest(manifest,
-						 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n"
-						 "\"Files\": [");
+						 "{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n"
+						 "\"System-Identifier\": " UINT64_FORMAT ",\n"
+						 "\"Files\": [",
+						 GetSystemIdentifier());
 }
 
 /*

src/backend/backup/basebackup_incremental.c

@@ -20,6 +20,7 @@
 #include "postgres.h"
 
 #include "access/timeline.h"
+#include "access/xlog.h"
 #include "backup/basebackup_incremental.h"
 #include "backup/walsummary.h"
 #include "common/blkreftable.h"
@@ -113,6 +114,10 @@ struct IncrementalBackupInfo
 	BlockRefTable *brtab;
 };
 
+static void manifest_process_version(JsonManifestParseContext *context,
+									 int manifest_version);
+static void manifest_process_system_identifier(JsonManifestParseContext *context,
+											   uint64 manifest_system_identifier);
 static void manifest_process_file(JsonManifestParseContext *context,
 								  char *pathname,
 								  size_t size,
@@ -199,6 +204,8 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib)
 
 	/* Parse the manifest. */
 	context.private_data = ib;
+	context.version_cb = manifest_process_version;
+	context.system_identifier_cb = manifest_process_system_identifier;
 	context.per_file_cb = manifest_process_file;
 	context.per_wal_range_cb = manifest_process_wal_range;
 	context.error_cb = manifest_report_error;
@@ -927,6 +934,39 @@ hash_string_pointer(const char *s)
 	return hash_bytes(ss, strlen(s));
 }
 
+/*
+ * This callback to validate the manifest version for incremental backup.
+ */
+static void
+manifest_process_version(JsonManifestParseContext *context,
+						 int manifest_version)
+{
+	/* Incremental backups don't work with manifest version 1 */
+	if (manifest_version == 1)
+		context->error_cb(context,
+						  "backup manifest version 1 does not support incremental backup");
+}
+
+/*
+ * This callback to validate the manifest system identifier against the current
+ * database server.
+ */
+static void
+manifest_process_system_identifier(JsonManifestParseContext *context,
+								   uint64 manifest_system_identifier)
+{
+	uint64		system_identifier;
+
+	/* Get system identifier of current system */
+	system_identifier = GetSystemIdentifier();
+
+	if (manifest_system_identifier != system_identifier)
+		context->error_cb(context,
+						  "manifest system identifier is %llu, but database system identifier is %llu",
+						  (unsigned long long) manifest_system_identifier,
+						  (unsigned long long) system_identifier);
+}
+
 /*
  * This callback is invoked for each file mentioned in the backup manifest.
  *

src/bin/pg_basebackup/t/010_pg_basebackup.pl

@@ -965,4 +965,20 @@
 my @dst_tblspc = glob "$backupdir/pg_tblspc/$tblspc_oid/PG_*";
 is(@dst_tblspc, 1, 'tblspc directory copied');
 
+# Can't take backup with referring manifest of different cluster
+#
+# Set up another new database instance with force initdb option. We don't want
+# to initializing database system by copying initdb template for this, because
+# we want it to be a separate cluster with a different system ID.
+my $node2 = PostgreSQL::Test::Cluster->new('node2');
+$node2->init(force_initdb => 1, has_archiving => 1, allows_streaming => 1);
+$node2->append_conf('postgresql.conf', 'summarize_wal = on');
+$node2->start;
+
+$node2->command_fails_like(
+	[ @pg_basebackup_defs, '-D', "$tempdir" . '/diff_sysid',
+		'--incremental', "$backupdir" . '/backup_manifest' ],
+	qr/manifest system identifier is .*, but database system identifier is/,
+	"pg_basebackup fails with different database system manifest");
+
 done_testing();

src/bin/pg_combinebackup/load_manifest.c

@@ -50,6 +50,10 @@ static uint32 hash_string_pointer(char *s);
 #define SH_DEFINE
 #include "lib/simplehash.h"
 
+static void combinebackup_version_cb(JsonManifestParseContext *context,
+									 int manifest_version);
+static void combinebackup_system_identifier_cb(JsonManifestParseContext *context,
+											   uint64 manifest_system_identifier);
 static void combinebackup_per_file_cb(JsonManifestParseContext *context,
 									  char *pathname, size_t size,
 									  pg_checksum_type checksum_type,
@@ -153,6 +157,8 @@ load_backup_manifest(char *backup_directory)
 	result = pg_malloc0(sizeof(manifest_data));
 	result->files = ht;
 	context.private_data = result;
+	context.version_cb = combinebackup_version_cb;
+	context.system_identifier_cb = combinebackup_system_identifier_cb;
 	context.per_file_cb = combinebackup_per_file_cb;
 	context.per_wal_range_cb = combinebackup_per_wal_range_cb;
 	context.error_cb = report_manifest_error;
@@ -181,6 +187,31 @@ report_manifest_error(JsonManifestParseContext *context, const char *fmt,...)
 	exit(1);
 }
 
+/*
+ * This callback to validate the manifest version number for incremental backup.
+ */
+static void
+combinebackup_version_cb(JsonManifestParseContext *context,
+						 int manifest_version)
+{
+	/* Incremental backups supported on manifest version 2 or later */
+	if (manifest_version == 1)
+		pg_fatal("backup manifest version 1 does not support incremental backup");
+}
+
+/*
+ * Record system identifier extracted from the backup manifest.
+ */
+static void
+combinebackup_system_identifier_cb(JsonManifestParseContext *context,
+								   uint64 manifest_system_identifier)
+{
+	manifest_data *manifest = context->private_data;
+
+	/* Validation will be at the later stage */
+	manifest->system_identifier = manifest_system_identifier;
+}
+
 /*
  * Record details extracted from the backup manifest for one file.
  */

src/bin/pg_combinebackup/load_manifest.h

@@ -55,6 +55,7 @@ typedef struct manifest_wal_range
  */
 typedef struct manifest_data
 {
+	uint64		system_identifier;
 	manifest_files_hash *files;
 	manifest_wal_range *first_wal_range;
 	manifest_wal_range *last_wal_range;

src/bin/pg_combinebackup/pg_combinebackup.c

@@ -92,7 +92,7 @@ cb_cleanup_dir *cleanup_dir_list = NULL;
 
 static void add_tablespace_mapping(cb_options *opt, char *arg);
 static StringInfo check_backup_label_files(int n_backups, char **backup_dirs);
-static void check_control_files(int n_backups, char **backup_dirs);
+static uint64 check_control_files(int n_backups, char **backup_dirs);
 static void check_input_dir_permissions(char *dir);
 static void cleanup_directories_atexit(void);
 static void create_output_directory(char *dirname, cb_options *opt);
@@ -134,11 +134,13 @@ main(int argc, char *argv[])
 
 	const char *progname;
 	char	   *last_input_dir;
+	int			i;
 	int			optindex;
 	int			c;
 	int			n_backups;
 	int			n_prior_backups;
 	int			version;
+	uint64		system_identifier;
 	char	  **prior_backup_dirs;
 	cb_options	opt;
 	cb_tablespace *tablespaces;
@@ -216,7 +218,7 @@ main(int argc, char *argv[])
 
 	/* Sanity-check control files. */
 	n_backups = argc - optind;
-	check_control_files(n_backups, argv + optind);
+	system_identifier = check_control_files(n_backups, argv + optind);
 
 	/* Sanity-check backup_label files, and get the contents of the last one. */
 	last_backup_label = check_backup_label_files(n_backups, argv + optind);
@@ -231,6 +233,26 @@ main(int argc, char *argv[])
 	/* Load backup manifests. */
 	manifests = load_backup_manifests(n_backups, prior_backup_dirs);
 
+	/*
+	 * Validate the manifest system identifier against the backup system
+	 * identifier.
+	 */
+	for (i = 0; i < n_backups; i++)
+	{
+		if (manifests[i] &&
+			manifests[i]->system_identifier != system_identifier)
+		{
+			char	   *controlpath;
+
+			controlpath = psprintf("%s/%s", prior_backup_dirs[i], "global/pg_control");
+
+			pg_fatal("%s: manifest system identifier is %llu, but control file has %llu",
+					 controlpath,
+					 (unsigned long long) manifests[i]->system_identifier,
+					 (unsigned long long) system_identifier);
+		}
+	}
+
 	/* Figure out which tablespaces are going to be included in the output. */
 	last_input_dir = argv[argc - 1];
 	check_input_dir_permissions(last_input_dir);
@@ -256,7 +278,7 @@ main(int argc, char *argv[])
 	/* If we need to write a backup_manifest, prepare to do so. */
 	if (!opt.dry_run && !opt.no_manifest)
 	{
-		mwriter = create_manifest_writer(opt.output);
+		mwriter = create_manifest_writer(opt.output, system_identifier);
 
 		/*
 		 * Verify that we have a backup manifest for the final backup; else we
@@ -517,9 +539,9 @@ check_backup_label_files(int n_backups, char **backup_dirs)
 }
 
 /*
- * Sanity check control files.
+ * Sanity check control files and return system_identifier.
  */
-static void
+static uint64
 check_control_files(int n_backups, char **backup_dirs)
 {
 	int			i;
@@ -564,6 +586,8 @@ check_control_files(int n_backups, char **backup_dirs)
 	 */
 	pg_log_debug("system identifier is %llu",
 				 (unsigned long long) system_identifier);
+
+	return system_identifier;
 }
 
 /*

src/bin/pg_combinebackup/t/005_integrity.pl

@@ -8,6 +8,7 @@
 use warnings FATAL => 'all';
 use File::Compare;
 use File::Path qw(rmtree);
+use File::Copy;
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
@@ -79,6 +80,19 @@
 	qr/expected system identifier.*but found/,
 	"can't combine backups from different nodes");
 
+# Can't combine when different manifest system identifier
+rename("$backup2path/backup_manifest", "$backup2path/backup_manifest.orig")
+  or die "could not move $backup2path/backup_manifest";
+copy("$backupother2path/backup_manifest", "$backup2path/backup_manifest")
+  or die "could not copy $backupother2path/backup_manifest";
+$node1->command_fails_like(
+	[ 'pg_combinebackup', $backup1path, $backup2path, $backup3path, '-o', $resultpath ],
+	qr/ manifest system identifier is .*, but control file has /,
+	"can't combine backups with different manifest system identifier ");
+# Restore the backup state
+move("$backup2path/backup_manifest.orig", "$backup2path/backup_manifest")
+  or die "could not move $backup2path/backup_manifest";
+
 # Can't omit a required backup.
 $node1->command_fails_like(
 	[ 'pg_combinebackup', $backup1path, $backup3path, '-o', $resultpath ],

src/bin/pg_combinebackup/write_manifest.c

@@ -45,7 +45,7 @@ static size_t hex_encode(const uint8 *src, size_t len, char *dst);
  * in the specified directory.
  */
 manifest_writer *
-create_manifest_writer(char *directory)
+create_manifest_writer(char *directory, uint64 system_identifier)
 {
 	manifest_writer *mwriter = pg_malloc(sizeof(manifest_writer));
 
@@ -57,8 +57,10 @@ create_manifest_writer(char *directory)
 	pg_checksum_init(&mwriter->manifest_ctx, CHECKSUM_TYPE_SHA256);
 
 	appendStringInfo(&mwriter->buf,
-					 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n"
-					 "\"Files\": [");
+					 "{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n"
+					 "\"System-Identifier\": " UINT64_FORMAT ",\n"
+					 "\"Files\": [",
+					 system_identifier);
 
 	return mwriter;
 }

src/bin/pg_combinebackup/write_manifest.h

@@ -19,7 +19,8 @@ struct manifest_wal_range;
 struct manifest_writer;
 typedef struct manifest_writer manifest_writer;
 
-extern manifest_writer *create_manifest_writer(char *directory);
+extern manifest_writer *create_manifest_writer(char *directory,
+											   uint64 system_identifier);
 extern void add_file_to_manifest(manifest_writer *mwriter,
 								 const char *manifest_path,
 								 size_t size, time_t mtime,