On Fri, Mar 1, 2024 at 11:28 AM Michael Paquier <mich...@paquier.xyz> wrote:
> On Mon, Feb 19, 2024 at 12:06:19PM +0530, Amul Sul wrote: > > Agreed, now they will have an error as _could not read file "<DataDir>": > Is > > a directory_. But, IIUC, that what usually happens with the dev version, > and > > the extension needs to be updated for compatibility with the newer > version for > > the same reason. > > I was reading through the remaining pieces of the patch set, and are > you sure that there is a need for 0002 at all? The only reason why > get_dir_controlfile() is introduced is to be able to get the contents > of a control file with a full path to it, and not a data folder. Now, > if I look closely, with 0002~0004 applied, the only two callers of > get_controlfile() are pg_combinebackup.c and pg_verifybackup.c. Both > of them have an access to the backup directories, which point to the > root of the data folder. pg_combinebackup can continue to use > backup_dirs[i]. pg_verifybackup has an access to the backup directory > in the context data, if I'm reading this right, so you could just use > that in verify_system_identifier(). > Yes, you are correct. Both the current caller of get_controlfile() has access to the root directory. I have dropped the 0002 patch -- I don't have a very strong opinion to refactor get_controlfile() apart from saying that it might be good to have both versions :) . Regards, Amul
From 6c54ab995d08d1843acaad71b2f8161019c72295 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 v9 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 ae8c18f3737..8561678a7d0 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 5166d2b1707f16933a8b51066d97aa1145ee8fbb Mon Sep 17 00:00:00 2001 From: Amul Sul <amul.sul@enterprisedb.com> Date: Mon, 4 Mar 2024 10:46:15 +0530 Subject: [PATCH v9 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 | 16 +++- doc/src/sgml/ref/pg_verifybackup.sgml | 7 +- src/backend/backup/backup_manifest.c | 7 +- src/backend/backup/basebackup_incremental.c | 39 +++++++++ src/bin/pg_basebackup/t/010_pg_basebackup.pl | 16 ++++ 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 | 85 ++++++++++++++++++- src/bin/pg_verifybackup/t/003_corruption.pl | 26 +++++- src/bin/pg_verifybackup/t/005_bad_manifest.pl | 17 +++- src/common/parse_manifest.c | 82 +++++++++++++++++- src/include/common/parse_manifest.h | 6 ++ 16 files changed, 370 insertions(+), 24 deletions(-) diff --git a/doc/src/sgml/backup-manifest.sgml b/doc/src/sgml/backup-manifest.sgml index 771be1310a1..c43af36dc6f 100644 --- a/doc/src/sgml/backup-manifest.sgml +++ b/doc/src/sgml/backup-manifest.sgml @@ -37,7 +37,21 @@ <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 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 + <literal>2</literal> and later. </para> </listitem> </varlistentry> diff --git a/doc/src/sgml/ref/pg_verifybackup.sgml b/doc/src/sgml/ref/pg_verifybackup.sgml index 36335e0a188..a3f167f9f6e 100644 --- a/doc/src/sgml/ref/pg_verifybackup.sgml +++ b/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> diff --git a/src/backend/backup/backup_manifest.c b/src/backend/backup/backup_manifest.c index 2c34e597523..0f31ae72e78 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 0919b85b442..0fa3d47b72f 100644 --- a/src/backend/backup/basebackup_incremental.c +++ b/src/backend/backup/basebackup_incremental.c @@ -113,6 +113,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 +203,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; @@ -911,6 +917,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 977ced71f83..d3c83f26e4b 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -965,4 +965,20 @@ $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 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(); diff --git a/src/bin/pg_combinebackup/load_manifest.c b/src/bin/pg_combinebackup/load_manifest.c index 2b8e74fcf38..4eae7f61efc 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 9163e071afd..8a5a70e4477 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 31ead7f4058..eb50bf4e58a 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 5dc71ddcf85..5f74e5bc58e 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; @@ -79,6 +80,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 01deb82cc9f..7a2065e1db7 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 de0f742779f..ebc4f9441ad 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 8561678a7d0..4ce211a35b2 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, @@ -375,9 +383,7 @@ main(int argc, char **argv) } /* - * Parse a manifest file. Construct a hash table with information about - * all the files it mentions, and a linked list of all the WAL ranges it - * mentions. + * Parse a manifest file and return a data structure describing the contents. */ static manifest_data * parse_manifest_file(char *manifest_path) @@ -432,6 +438,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 +469,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 +551,43 @@ 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); +} + /* * Verify one directory. * @@ -650,6 +721,14 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath) m->bad = true; } + /* + * Validate the manifest system identifier, not available in manifest + * version 1. + */ + if (context->manifest->version != 1 && + strcmp(relpath, "global/pg_control") == 0) + verify_system_identifier(context); + /* Update statistics for progress report, if necessary */ if (show_progress && !skip_checksums && should_verify_checksum(m)) total_size += m->size; diff --git a/src/bin/pg_verifybackup/t/003_corruption.pl b/src/bin/pg_verifybackup/t/003_corruption.pl index 11bd5770818..36d032288fe 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,24 @@ 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 = PostgreSQL::Test::Cluster->new('node'); + $node->init(force_initdb => 1, 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/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl index e278ccea5a2..f95b45a7230 100644 --- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl +++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl @@ -29,10 +29,14 @@ test_parse_error('expected version indicator', <<EOM); {"not-expected": 1} EOM -test_parse_error('unexpected manifest version', <<EOM); +test_parse_error('manifest version not an integer', <<EOM); {"PostgreSQL-Backup-Manifest-Version": "phooey"} EOM +test_parse_error('unexpected manifest version', <<EOM); +{"PostgreSQL-Backup-Manifest-Version": 9876599} +EOM + test_parse_error('unexpected scalar', <<EOM); {"PostgreSQL-Backup-Manifest-Version": 1, "Files": true} EOM @@ -156,6 +160,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 92a97714f36..7e3682f75b0 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,60 @@ 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); + + /* Parse version. */ + version = strtoi64(parse->manifest_version, &ep, 10); + if (*ep) + json_manifest_parse_failure(parse->context, + "manifest version not an integer"); + + if (version != 1 && version != 2) + json_manifest_parse_failure(parse->context, + "unexpected manifest version"); + + /* 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 f74be0db35f..78b052c045b 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