On Wed, Jan 24, 2024 at 10:53 PM Robert Haas <robertmh...@gmail.com> wrote:
> On Mon, Jan 22, 2024 at 2:22 AM Amul Sul <sula...@gmail.com> wrote:
> > Thinking a bit more on this, I realized parse_manifest_file() has many
> out
> > parameters. Instead parse_manifest_file() should simply return manifest
> data
> > like load_backup_manifest(). Attached 0001 patch doing the same, and
> removed
> > parser_context structure, and added manifest_data, and did the required
> > adjustments to pg_verifybackup code.
>
> InitializeBackupManifest(&manifest, opt->manifest,
> -
> opt->manifest_checksum_type);
> +
> opt->manifest_checksum_type,
> +
> GetSystemIdentifier());
>
> InitializeBackupManifest() can just call GetSystemIdentifier() itself,
> instead of passing another parameter, I think.
>
Ok.
>
> + if (manifest_version == 1)
> + context->error_cb(context,
> + "%s: backup manifest
> doesn't support incremental changes",
> +
> private_context->backup_directory);
>
> I think this is weird. First, there doesn't seem to be any reason to
> bounce through error_cb() here. You could just call pg_fatal(), as we
> do elsewhere in this file. Second, there doesn't seem to be any need
> to include the backup directory in this error message. We include the
> file name (not the directory name) in errors that pertain to the file
> itself, like if we can't open or read it. But we don't do that for
> semantic errors about the manifest contents (cf.
> combinebackup_per_file_cb). This file would need a lot fewer charges
> if you didn't feed the backup directory name through here. Third, the
> error message is not well-chosen, because a user who looks at it won't
> understand WHY the manifest doesn't support incremental changes. I
> suggest "backup manifest version 1 does not support incremental
> backup".
>
> + /* Incremental backups supported on manifest version 2 or later */
> + if (manifest_version == 1)
> + context->error_cb(context,
> + "incremental backups
> cannot be taken for this backup");
>
> Let's use the same error message as in the previous case here also.
>
Ok.
> + for (i = 0; i < n_backups; i++)
> + {
> + if (manifests[i]->system_identifier != system_identifier)
> + {
> + char *controlpath;
> +
> + controlpath = psprintf("%s/%s",
> prior_backup_dirs[i], "global/pg_control");
> +
> + pg_fatal("manifest is from different database
> system: manifest database system identifier is %llu, %s system
> identifier is %llu",
> + (unsigned long long)
> manifests[i]->system_identifier,
> + controlpath,
> + (unsigned long long)
> system_identifier);
> + }
> + }
>
> check_control_files() already verifies that all of the control files
> contain the same system identifier as each other, so what we're really
> checking here is that the backup manifest in each directory has the
> same system identifier as the control file in that same directory. One
> problem is that backup manifests are optional here, as per the comment
> in load_backup_manifests(), so you need to skip over NULL entries
> cleanly to avoid seg faulting if one is missing. I also think the
> error message should be changed. How about "%s: manifest system
> identifier is %llu, but control file has %llu"?
>
Ok.
> + context->error_cb(context,
> + "manifest is from
> different database system: manifest database system identifier is
> %llu, pg_control database system identifier is %llu",
> + (unsigned long long)
> manifest_system_identifier,
> + (unsigned long long)
> system_identifier);
>
> And here, while I'm kibitzing, how about "manifest system identifier
> is %llu, but this system's identifier is %llu"?
>
I used "database system identifier" instead of "this system's identifier "
like
we are using in WalReceiverMain() and libpqrcv_identify_system().
> - qr/could not open directory/,
> + qr/could not open file/,
>
> I don't think that the expected error message here should be changing.
> Does it really, with the latest patch version? Why? Can we fix that?
>
Because, we were trying to access pg_control to check the system identifier
before any other backup directory/file validation.
> + else if (!parse->saw_system_identifier_field &&
> +
> strcmp(parse->manifest_version, "1") != 0)
>
> I don't think this has any business testing the manifest version.
> That's something to sort out at some later stage.
>
That is for backward compatibility, otherwise, we would have an "expected
system identifier" error for manifest version 1.
Thank you for the review-comments, updated version attached.
Regards,
Amul
From 395ffe7fef21f18883d97888d1edb01478fe1fb6 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Mon, 22 Jan 2024 10:45:19 +0530
Subject: [PATCH v5 1/2] pg_verifybackup: code refactor
Rename parser_context to manifest_data and make parse_manifest_file
return that instead of out-argument.
---
src/bin/pg_verifybackup/pg_verifybackup.c | 75 ++++++++++-------------
1 file changed, 34 insertions(+), 41 deletions(-)
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index ae8c18f373..8561678a7d 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -94,31 +94,28 @@ typedef struct manifest_wal_range
} manifest_wal_range;
/*
- * Details we need in callbacks that occur while parsing a backup manifest.
+ * All the data parsed from a backup_manifest file.
*/
-typedef struct parser_context
+typedef struct manifest_data
{
- manifest_files_hash *ht;
+ manifest_files_hash *files;
manifest_wal_range *first_wal_range;
manifest_wal_range *last_wal_range;
-} parser_context;
+} manifest_data;
/*
* All of the context information we need while checking a backup manifest.
*/
typedef struct verifier_context
{
- manifest_files_hash *ht;
+ manifest_data *manifest;
char *backup_directory;
SimpleStringList ignore_list;
bool exit_on_error;
bool saw_any_error;
} verifier_context;
-static void parse_manifest_file(char *manifest_path,
- manifest_files_hash **ht_p,
- manifest_wal_range **first_wal_range_p);
-
+static manifest_data *parse_manifest_file(char *manifest_path);
static void verifybackup_per_file_cb(JsonManifestParseContext *context,
char *pathname, size_t size,
pg_checksum_type checksum_type,
@@ -142,8 +139,7 @@ static void verify_file_checksum(verifier_context *context,
manifest_file *m, char *fullpath);
static void parse_required_wal(verifier_context *context,
char *pg_waldump_path,
- char *wal_directory,
- manifest_wal_range *first_wal_range);
+ char *wal_directory);
static void report_backup_error(verifier_context *context,
const char *pg_restrict fmt,...)
@@ -185,7 +181,6 @@ main(int argc, char **argv)
int c;
verifier_context context;
- manifest_wal_range *first_wal_range;
char *manifest_path = NULL;
bool no_parse_wal = false;
bool quiet = false;
@@ -338,7 +333,7 @@ main(int argc, char **argv)
* the manifest as fatal; there doesn't seem to be much point in trying to
* verify the backup directory against a corrupted manifest.
*/
- parse_manifest_file(manifest_path, &context.ht, &first_wal_range);
+ context.manifest = parse_manifest_file(manifest_path);
/*
* Now scan the files in the backup directory. At this stage, we verify
@@ -367,8 +362,7 @@ main(int argc, char **argv)
* not to do so.
*/
if (!no_parse_wal)
- parse_required_wal(&context, pg_waldump_path,
- wal_directory, first_wal_range);
+ parse_required_wal(&context, pg_waldump_path, wal_directory);
/*
* If everything looks OK, tell the user this, unless we were asked to
@@ -385,9 +379,8 @@ main(int argc, char **argv)
* all the files it mentions, and a linked list of all the WAL ranges it
* mentions.
*/
-static void
-parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p,
- manifest_wal_range **first_wal_range_p)
+static manifest_data *
+parse_manifest_file(char *manifest_path)
{
int fd;
struct stat statbuf;
@@ -396,8 +389,8 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p,
manifest_files_hash *ht;
char *buffer;
int rc;
- parser_context private_context;
JsonManifestParseContext context;
+ manifest_data *result;
/* Open the manifest file. */
if ((fd = open(manifest_path, O_RDONLY | PG_BINARY, 0)) < 0)
@@ -436,10 +429,9 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p,
close(fd);
/* Parse the manifest. */
- private_context.ht = ht;
- private_context.first_wal_range = NULL;
- private_context.last_wal_range = NULL;
- context.private_data = &private_context;
+ result = pg_malloc0(sizeof(manifest_data));
+ result->files = ht;
+ context.private_data = result;
context.per_file_cb = verifybackup_per_file_cb;
context.per_wal_range_cb = verifybackup_per_wal_range_cb;
context.error_cb = report_manifest_error;
@@ -448,9 +440,7 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p,
/* Done with the buffer. */
pfree(buffer);
- /* Return the file hash table and WAL range list we constructed. */
- *ht_p = ht;
- *first_wal_range_p = private_context.first_wal_range;
+ return result;
}
/*
@@ -480,8 +470,8 @@ verifybackup_per_file_cb(JsonManifestParseContext *context,
pg_checksum_type checksum_type,
int checksum_length, uint8 *checksum_payload)
{
- parser_context *pcxt = context->private_data;
- manifest_files_hash *ht = pcxt->ht;
+ manifest_data *manifest = context->private_data;
+ manifest_files_hash *ht = manifest->files;
manifest_file *m;
bool found;
@@ -508,7 +498,7 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context,
TimeLineID tli,
XLogRecPtr start_lsn, XLogRecPtr end_lsn)
{
- parser_context *pcxt = context->private_data;
+ manifest_data *manifest = context->private_data;
manifest_wal_range *range;
/* Allocate and initialize a struct describing this WAL range. */
@@ -516,15 +506,15 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context,
range->tli = tli;
range->start_lsn = start_lsn;
range->end_lsn = end_lsn;
- range->prev = pcxt->last_wal_range;
+ range->prev = manifest->last_wal_range;
range->next = NULL;
/* Add it to the end of the list. */
- if (pcxt->first_wal_range == NULL)
- pcxt->first_wal_range = range;
+ if (manifest->first_wal_range == NULL)
+ manifest->first_wal_range = range;
else
- pcxt->last_wal_range->next = range;
- pcxt->last_wal_range = range;
+ manifest->last_wal_range->next = range;
+ manifest->last_wal_range = range;
}
/*
@@ -639,7 +629,7 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath)
}
/* Check whether there's an entry in the manifest hash. */
- m = manifest_files_lookup(context->ht, relpath);
+ m = manifest_files_lookup(context->manifest->files, relpath);
if (m == NULL)
{
report_backup_error(context,
@@ -679,11 +669,12 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath)
static void
report_extra_backup_files(verifier_context *context)
{
+ manifest_data *manifest = context->manifest;
manifest_files_iterator it;
manifest_file *m;
- manifest_files_start_iterate(context->ht, &it);
- while ((m = manifest_files_iterate(context->ht, &it)) != NULL)
+ manifest_files_start_iterate(manifest->files, &it);
+ while ((m = manifest_files_iterate(manifest->files, &it)) != NULL)
if (!m->matched && !should_ignore_relpath(context, m->pathname))
report_backup_error(context,
"\"%s\" is present in the manifest but not on disk",
@@ -698,13 +689,14 @@ report_extra_backup_files(verifier_context *context)
static void
verify_backup_checksums(verifier_context *context)
{
+ manifest_data *manifest = context->manifest;
manifest_files_iterator it;
manifest_file *m;
progress_report(false);
- manifest_files_start_iterate(context->ht, &it);
- while ((m = manifest_files_iterate(context->ht, &it)) != NULL)
+ manifest_files_start_iterate(manifest->files, &it);
+ while ((m = manifest_files_iterate(manifest->files, &it)) != NULL)
{
if (should_verify_checksum(m) &&
!should_ignore_relpath(context, m->pathname))
@@ -833,9 +825,10 @@ verify_file_checksum(verifier_context *context, manifest_file *m,
*/
static void
parse_required_wal(verifier_context *context, char *pg_waldump_path,
- char *wal_directory, manifest_wal_range *first_wal_range)
+ char *wal_directory)
{
- manifest_wal_range *this_wal_range = first_wal_range;
+ manifest_data *manifest = context->manifest;
+ manifest_wal_range *this_wal_range = manifest->first_wal_range;
while (this_wal_range != NULL)
{
--
2.18.0
From 8f7e9348f3c454d47abc8afa5b75d0b499d4d8dd Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Mon, 22 Jan 2024 11:19:30 +0530
Subject: [PATCH v5 2/2] Add system identifier to backup manifest
The backup manifest will be having a new item as "System-Identifier"
and its value is from "ControlFileData.system_identifier" when
manifest generated. This help identify the correct database server
and/or backup while taking subsequent backup.
---
doc/src/sgml/backup-manifest.sgml | 15 +++-
doc/src/sgml/ref/pg_verifybackup.sgml | 3 +-
src/backend/backup/backup_manifest.c | 7 +-
src/backend/backup/basebackup_incremental.c | 39 +++++++++
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 20 +++++
src/bin/pg_combinebackup/load_manifest.c | 33 +++++++-
src/bin/pg_combinebackup/load_manifest.h | 1 +
src/bin/pg_combinebackup/pg_combinebackup.c | 34 ++++++--
src/bin/pg_combinebackup/t/005_integrity.pl | 14 ++++
src/bin/pg_combinebackup/write_manifest.c | 8 +-
src/bin/pg_combinebackup/write_manifest.h | 3 +-
src/bin/pg_verifybackup/pg_verifybackup.c | 81 ++++++++++++++++++
src/bin/pg_verifybackup/t/003_corruption.pl | 31 ++++++-
src/bin/pg_verifybackup/t/004_options.pl | 2 +-
src/bin/pg_verifybackup/t/005_bad_manifest.pl | 11 +++
src/common/parse_manifest.c | 83 ++++++++++++++++++-
src/include/common/parse_manifest.h | 6 ++
17 files changed, 372 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/backup-manifest.sgml b/doc/src/sgml/backup-manifest.sgml
index 771be1310a..a67462e3eb 100644
--- a/doc/src/sgml/backup-manifest.sgml
+++ b/doc/src/sgml/backup-manifest.sgml
@@ -37,7 +37,20 @@
<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> 17, it is 2; in older versions,
+ it is 1.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>System-Identifier</literal></term>
+ <listitem>
+ <para>
+ The associated integer value is an unique system identifier to ensure
+ file match up with the installation that produced them. Available only
+ when <literal>PostgreSQL-Backup-Manifest-Version</literal> is 2 and later.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_verifybackup.sgml b/doc/src/sgml/ref/pg_verifybackup.sgml
index 36335e0a18..3051517d92 100644
--- a/doc/src/sgml/ref/pg_verifybackup.sgml
+++ b/doc/src/sgml/ref/pg_verifybackup.sgml
@@ -53,7 +53,8 @@ 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
+ does not exist, cannot be read, is malformed, fails to match the system
+ identifier with pg_control of the backup directory or fails verification
against its own internal checksum, <literal>pg_verifybackup</literal>
will terminate with a fatal error.
</para>
diff --git a/src/backend/backup/backup_manifest.c b/src/backend/backup/backup_manifest.c
index 2c34e59752..0f31ae72e7 100644
--- a/src/backend/backup/backup_manifest.c
+++ b/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 "libpq/libpq.h"
@@ -79,8 +80,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());
}
/*
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 0504c465db..46e7d90438 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -112,6 +112,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,
@@ -198,6 +202,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;
@@ -910,6 +916,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 supported on manifest version 2 or later */
+ 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.
*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 42a09d0da3..0a8814a5a1 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -949,4 +949,24 @@ $backupdir = $node->backup_dir . '/backup3';
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. We don't want to use the cached
+# INITDB_TEMPLATE for this, because we want it to be a separate cluster
+# with a different system ID.
+my $node2;
+{
+ local $ENV{'INITDB_TEMPLATE'} = undef;
+
+ $node2 = PostgreSQL::Test::Cluster->new('node2');
+ $node2->init(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();
diff --git a/src/bin/pg_combinebackup/load_manifest.c b/src/bin/pg_combinebackup/load_manifest.c
index 2b8e74fcf3..4eae7f61ef 100644
--- a/src/bin/pg_combinebackup/load_manifest.c
+++ b/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,
@@ -103,8 +107,8 @@ load_backup_manifest(char *backup_directory)
manifest_files_hash *ht;
char *buffer;
int rc;
- JsonManifestParseContext context;
manifest_data *result;
+ JsonManifestParseContext context;
/* Open the manifest file. */
snprintf(pathname, MAXPGPATH, "%s/backup_manifest", backup_directory);
@@ -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.
*/
diff --git a/src/bin/pg_combinebackup/load_manifest.h b/src/bin/pg_combinebackup/load_manifest.h
index 9163e071af..8a5a70e447 100644
--- a/src/bin/pg_combinebackup/load_manifest.h
+++ b/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;
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 31ead7f405..eb50bf4e58 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/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;
}
/*
diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl
index 3b445d0e30..9f8efe2f83 100644
--- a/src/bin/pg_combinebackup/t/005_integrity.pl
+++ b/src/bin/pg_combinebackup/t/005_integrity.pl
@@ -8,6 +8,7 @@ use strict;
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;
@@ -85,6 +86,19 @@ $node1->command_fails_like(
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 BAIL_OUT "could not move $backup2path/backup_manifest";
+copy("$backupother2path/backup_manifest", "$backup2path/backup_manifest")
+ or BAIL_OUT "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 BAIL_OUT "could not move $backup2path/backup_manifest";
+
# Can't omit a required backup.
$node1->command_fails_like(
[ 'pg_combinebackup', $backup1path, $backup3path, '-o', $resultpath ],
diff --git a/src/bin/pg_combinebackup/write_manifest.c b/src/bin/pg_combinebackup/write_manifest.c
index 01deb82cc9..7a2065e1db 100644
--- a/src/bin/pg_combinebackup/write_manifest.c
+++ b/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;
}
diff --git a/src/bin/pg_combinebackup/write_manifest.h b/src/bin/pg_combinebackup/write_manifest.h
index de0f742779..ebc4f9441a 100644
--- a/src/bin/pg_combinebackup/write_manifest.h
+++ b/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,
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 8561678a7d..9be7af5b6d 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -18,6 +18,7 @@
#include <sys/stat.h>
#include <time.h>
+#include "common/controldata_utils.h"
#include "common/hashfn.h"
#include "common/logging.h"
#include "common/parse_manifest.h"
@@ -98,6 +99,8 @@ typedef struct manifest_wal_range
*/
typedef struct manifest_data
{
+ int version;
+ uint64 system_identifier;
manifest_files_hash *files;
manifest_wal_range *first_wal_range;
manifest_wal_range *last_wal_range;
@@ -116,6 +119,10 @@ typedef struct verifier_context
} verifier_context;
static manifest_data *parse_manifest_file(char *manifest_path);
+static void verifybackup_version_cb(JsonManifestParseContext *context,
+ int manifest_version);
+static void verifybackup_system_identifier(JsonManifestParseContext *context,
+ uint64 manifest_system_identifier);
static void verifybackup_per_file_cb(JsonManifestParseContext *context,
char *pathname, size_t size,
pg_checksum_type checksum_type,
@@ -129,6 +136,7 @@ static void report_manifest_error(JsonManifestParseContext *context,
const char *fmt,...)
pg_attribute_printf(2, 3) pg_attribute_noreturn();
+static void verify_system_identifier(verifier_context *context);
static void verify_backup_directory(verifier_context *context,
char *relpath, char *fullpath);
static void verify_backup_file(verifier_context *context,
@@ -335,6 +343,13 @@ main(int argc, char **argv)
*/
context.manifest = parse_manifest_file(manifest_path);
+ /*
+ * Validate the manifest system identifier, not available in manifest
+ * version 1.
+ */
+ if (context.manifest->version != 1)
+ verify_system_identifier(&context);
+
/*
* Now scan the files in the backup directory. At this stage, we verify
* that every file on disk is present in the manifest and that the sizes
@@ -432,6 +447,8 @@ parse_manifest_file(char *manifest_path)
result = pg_malloc0(sizeof(manifest_data));
result->files = ht;
context.private_data = result;
+ context.version_cb = verifybackup_version_cb;
+ context.system_identifier_cb = verifybackup_system_identifier;
context.per_file_cb = verifybackup_per_file_cb;
context.per_wal_range_cb = verifybackup_per_wal_range_cb;
context.error_cb = report_manifest_error;
@@ -461,6 +478,32 @@ report_manifest_error(JsonManifestParseContext *context, const char *fmt,...)
exit(1);
}
+/*
+ * Record details extracted from the backup manifest.
+ */
+static void
+verifybackup_version_cb(JsonManifestParseContext *context,
+ int manifest_version)
+{
+ manifest_data *manifest = context->private_data;
+
+ /* Validation will be at the later stage */
+ manifest->version = manifest_version;
+}
+
+/*
+ * Record details extracted from the backup manifest.
+ */
+static void
+verifybackup_system_identifier(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.
*/
@@ -517,6 +560,44 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context,
manifest->last_wal_range = range;
}
+/*
+ * Sanity check control file of the backup and validate system identifier
+ * against manifest system identifier.
+ */
+static void
+verify_system_identifier(verifier_context *context)
+{
+ manifest_data *manifest = context->manifest;
+ char *backup_directory = context->backup_directory;
+ char *controlpath;
+ ControlFileData *control_file;
+ bool crc_ok;
+
+ controlpath = psprintf("%s/%s", backup_directory, "global/pg_control");
+ pg_log_debug("reading \"%s\"", controlpath);
+ control_file = get_controlfile(backup_directory, &crc_ok);
+
+ /* Control file contents not meaningful if CRC is bad. */
+ if (!crc_ok)
+ report_fatal_error("%s: CRC is incorrect", controlpath);
+
+ /* Can't interpret control file if not current version. */
+ if (control_file->pg_control_version != PG_CONTROL_VERSION)
+ report_fatal_error("%s: unexpected control file version",
+ controlpath);
+
+ /* System identifiers should match. */
+ if (manifest->system_identifier != control_file->system_identifier)
+ report_fatal_error("%s: manifest system identifier is %llu, but control file has %llu",
+ controlpath,
+ (unsigned long long) manifest->system_identifier,
+ (unsigned long long) control_file->system_identifier);
+
+ /* Release memory. */
+ pfree(control_file);
+ pfree(controlpath);
+}
+
/*
* Verify one directory.
*
diff --git a/src/bin/pg_verifybackup/t/003_corruption.pl b/src/bin/pg_verifybackup/t/003_corruption.pl
index 11bd577081..c83a869c41 100644
--- a/src/bin/pg_verifybackup/t/003_corruption.pl
+++ b/src/bin/pg_verifybackup/t/003_corruption.pl
@@ -6,6 +6,7 @@
use strict;
use warnings FATAL => 'all';
use File::Path qw(rmtree);
+use File::Copy;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
@@ -68,6 +69,11 @@ my @scenario = (
'mutilate' => \&mutilate_replace_file,
'fails_like' => qr/checksum mismatch for file/
},
+ {
+ 'name' => 'system_identifier',
+ 'mutilate' => \&mutilate_system_identifier,
+ 'fails_like' => qr/manifest system identifier is .*, but control file has/
+ },
{
'name' => 'bad_manifest',
'mutilate' => \&mutilate_bad_manifest,
@@ -216,7 +222,7 @@ sub mutilate_append_to_file
sub mutilate_truncate_file
{
my ($backup_path) = @_;
- my $pathname = "$backup_path/global/pg_control";
+ my $pathname = "$backup_path/pg_hba.conf";
open(my $fh, '>', $pathname) || die "open $pathname: $!";
close($fh);
return;
@@ -236,6 +242,29 @@ sub mutilate_replace_file
return;
}
+# Copy manifest of other backups to demonstrate the case where the wrong
+# manifest is referred
+sub mutilate_system_identifier
+{
+ my ($backup_path) = @_;
+
+ # Set up another new database instance with different system identifier and
+ # make backup
+ my $node;
+ {
+ local $ENV{'INITDB_TEMPLATE'} = undef;
+
+ $node = PostgreSQL::Test::Cluster->new('node');
+ $node->init(allows_streaming => 1);
+ $node->start;
+ }
+ $node->backup('backup2');
+ move($node->backup_dir.'/backup2/backup_manifest', $backup_path.'/backup_manifest')
+ or BAIL_OUT "could not copy manifest to $backup_path";
+ $node->teardown_node(fail_ok => 1);
+ return;
+}
+
# Corrupt the backup manifest.
sub mutilate_bad_manifest
{
diff --git a/src/bin/pg_verifybackup/t/004_options.pl b/src/bin/pg_verifybackup/t/004_options.pl
index 8ed2214408..970b7dcd56 100644
--- a/src/bin/pg_verifybackup/t/004_options.pl
+++ b/src/bin/pg_verifybackup/t/004_options.pl
@@ -111,7 +111,7 @@ command_fails_like(
'pg_verifybackup', '-m',
"$backup_path/backup_manifest", "$backup_path/fake"
],
- qr/could not open directory/,
+ qr/could not open file/,
'nonexistent backup directory');
done_testing();
diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
index e278ccea5a..7fac2fa836 100644
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -156,6 +156,17 @@ test_parse_error('expected at least 2 lines', <<EOM);
{"PostgreSQL-Backup-Manifest-Version": 1, "Files": [], "Manifest-Checksum": null}
EOM
+test_parse_error('unrecognized top-level field', <<EOM);
+{"PostgreSQL-Backup-Manifest-Version": 1,
+ "System-Identifier": "7320280665284567118"
+ "Manifest-Checksum": "eabdc9caf8a" }
+EOM
+
+test_parse_error('expected system identifier', <<EOM);
+{"PostgreSQL-Backup-Manifest-Version": 2,
+ "Manifest-Checksum": "eabdc9caf8a" }
+EOM
+
my $manifest_without_newline = <<EOM;
{"PostgreSQL-Backup-Manifest-Version": 1,
"Files": [],
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 92a97714f3..9c28ed3ef8 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -25,6 +25,7 @@ typedef enum
JM_EXPECT_TOPLEVEL_END,
JM_EXPECT_TOPLEVEL_FIELD,
JM_EXPECT_VERSION_VALUE,
+ JM_EXPECT_SYSTEM_IDENTIFIER_VALUE,
JM_EXPECT_FILES_START,
JM_EXPECT_FILES_NEXT,
JM_EXPECT_THIS_FILE_FIELD,
@@ -85,6 +86,9 @@ typedef struct
/* Miscellaneous other stuff. */
bool saw_version_field;
+ bool saw_system_identifier_field;
+ char *manifest_version;
+ char *manifest_system_identifier;
char *manifest_checksum;
} JsonManifestParseState;
@@ -96,6 +100,8 @@ static JsonParseErrorType json_manifest_object_field_start(void *state, char *fn
bool isnull);
static JsonParseErrorType json_manifest_scalar(void *state, char *token,
JsonTokenType tokentype);
+static void json_manifest_finalize_version(JsonManifestParseState *parse);
+static void json_manifest_finalize_system_identifier(JsonManifestParseState *parse);
static void json_manifest_finalize_file(JsonManifestParseState *parse);
static void json_manifest_finalize_wal_range(JsonManifestParseState *parse);
static void verify_manifest_checksum(JsonManifestParseState *parse,
@@ -128,6 +134,7 @@ json_parse_manifest(JsonManifestParseContext *context, char *buffer,
parse.context = context;
parse.state = JM_EXPECT_TOPLEVEL_START;
parse.saw_version_field = false;
+ parse.saw_system_identifier_field = false;
/* Create a JSON lexing context. */
lex = makeJsonLexContextCstringLen(NULL, buffer, size, PG_UTF8, true);
@@ -311,6 +318,16 @@ json_manifest_object_field_start(void *state, char *fname, bool isnull)
parse->saw_version_field = true;
break;
}
+ else if (!parse->saw_system_identifier_field &&
+ strcmp(parse->manifest_version, "1") != 0)
+ {
+ if (strcmp(fname, "System-Identifier") != 0)
+ json_manifest_parse_failure(parse->context,
+ "expected system identifier");
+ parse->state = JM_EXPECT_SYSTEM_IDENTIFIER_VALUE;
+ parse->saw_system_identifier_field = true;
+ break;
+ }
/* Is this the list of files? */
if (strcmp(fname, "Files") == 0)
@@ -404,9 +421,14 @@ json_manifest_scalar(void *state, char *token, JsonTokenType tokentype)
switch (parse->state)
{
case JM_EXPECT_VERSION_VALUE:
- if (strcmp(token, "1") != 0)
- json_manifest_parse_failure(parse->context,
- "unexpected manifest version");
+ parse->manifest_version = token;
+ json_manifest_finalize_version(parse);
+ parse->state = JM_EXPECT_TOPLEVEL_FIELD;
+ break;
+
+ case JM_EXPECT_SYSTEM_IDENTIFIER_VALUE:
+ parse->manifest_system_identifier = token;
+ json_manifest_finalize_system_identifier(parse);
parse->state = JM_EXPECT_TOPLEVEL_FIELD;
break;
@@ -464,6 +486,61 @@ json_manifest_scalar(void *state, char *token, JsonTokenType tokentype)
return JSON_SUCCESS;
}
+/*
+ * Do additional parsing and sanity-checking of the manifest version, and invoke
+ * the callback so that the caller can gets that detail and take actions
+ * accordingly. This happens for each manifest when the corresponding JSON
+ * object is completely parsed.
+ */
+static void
+json_manifest_finalize_version(JsonManifestParseState *parse)
+{
+ JsonManifestParseContext *context = parse->context;
+ int version;
+ char *ep;
+
+ Assert(parse->saw_version_field);
+
+ if (strcmp(parse->manifest_version, "1") != 0 &&
+ strcmp(parse->manifest_version, "2") != 0)
+ json_manifest_parse_failure(parse->context,
+ "unexpected manifest version");
+
+ /* Parse version. */
+ version = strtoi64(parse->manifest_version, &ep, 10);
+ if (*ep)
+ json_manifest_parse_failure(parse->context,
+ "manifest version not an integer");
+
+ /* Invoke the callback for version */
+ context->version_cb(context, version);
+}
+
+/*
+ * Do additional parsing and sanity-checking of the system identifier, and
+ * invoke the callback so that the caller can gets that detail and take actions
+ * accordingly. This happens for each manifest when the corresponding JSON
+ * object is completely parsed.
+ */
+static void
+json_manifest_finalize_system_identifier(JsonManifestParseState *parse)
+{
+ JsonManifestParseContext *context = parse->context;
+ uint64 system_identifier;
+ char *ep;
+
+ Assert(parse->saw_system_identifier_field);
+
+ /* Parse system identifier. */
+ system_identifier = strtou64(parse->manifest_system_identifier, &ep, 10);
+ if (*ep)
+ json_manifest_parse_failure(parse->context,
+ "manifest system identifier not an integer");
+
+ /* Invoke the callback for system identifier */
+ context->system_identifier_cb(context, system_identifier);
+}
+
/*
* Do additional parsing and sanity-checking of the details gathered for one
* file, and invoke the per-file callback so that the caller gets those
diff --git a/src/include/common/parse_manifest.h b/src/include/common/parse_manifest.h
index f74be0db35..78b052c045 100644
--- a/src/include/common/parse_manifest.h
+++ b/src/include/common/parse_manifest.h
@@ -21,6 +21,10 @@
struct JsonManifestParseContext;
typedef struct JsonManifestParseContext JsonManifestParseContext;
+typedef void (*json_manifest_version_callback) (JsonManifestParseContext *,
+ int manifest_version);
+typedef void (*json_manifest_system_identifier_callback) (JsonManifestParseContext *,
+ uint64 manifest_system_identifier);
typedef void (*json_manifest_per_file_callback) (JsonManifestParseContext *,
char *pathname,
size_t size, pg_checksum_type checksum_type,
@@ -35,6 +39,8 @@ typedef void (*json_manifest_error_callback) (JsonManifestParseContext *,
struct JsonManifestParseContext
{
void *private_data;
+ json_manifest_version_callback version_cb;
+ json_manifest_system_identifier_callback system_identifier_cb;
json_manifest_per_file_callback per_file_cb;
json_manifest_per_wal_range_callback per_wal_range_cb;
json_manifest_error_callback error_cb;
--
2.18.0
