On Mon, Feb 19, 2024 at 4:22 AM Michael Paquier <mich...@paquier.xyz> wrote:
> On Thu, Feb 15, 2024 at 05:41:46PM +0530, Robert Haas wrote:
> > On Thu, Feb 15, 2024 at 3:05 PM Amul Sul <sula...@gmail.com> wrote:
> > > Kindly have a look at the attached version.
> >
> > IMHO, 0001 looks fine, except probably the comment could be phrased a
> > bit more nicely.
>
> And the new option should be documented at the top of the init()
> routine for perldoc.
>
Added in the attached version.
> > That can be left for whoever commits this to
> > wordsmith. Michael, what are your plans?
>
> Not much, so feel free to not wait for me. I've just read through the
> patch because I like the idea/feature :)
>
Thank you, that helped a lot.
> 0002 seems like a reasonable approach, but there's a hunk in the wrong
> > patch: 0004 modifies pg_combinebackup's check_control_files to use
> > get_dir_controlfile() rather than git_controlfile(), but it looks to
> > me like that should be part of 0002.
>
Fixed in the attached version.
> I'm slightly concerned about 0002 that silently changes the meaning of
> get_controlfile(). That would impact extension code without people
> knowing about it when compiling, just when they run their stuff under
> 17~.
>
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.
Regards,
Amul
From 354014538b16579f005bd6f4ce771b1aa22b5e02 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 15 Feb 2024 12:10:23 +0530
Subject: [PATCH v8 1/4] Add option to force initdb in
PostgreSQL::Test::Cluster:init()
---
src/bin/pg_combinebackup/t/005_integrity.pl | 19 +++++++------------
src/test/perl/PostgreSQL/Test/Cluster.pm | 15 ++++++++++-----
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl
index 3b445d0e30f..5d425209211 100644
--- a/src/bin/pg_combinebackup/t/005_integrity.pl
+++ b/src/bin/pg_combinebackup/t/005_integrity.pl
@@ -18,18 +18,13 @@ $node1->init(has_archiving => 1, allows_streaming => 1);
$node1->append_conf('postgresql.conf', 'summarize_wal = on');
$node1->start;
-# 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;
-}
+# 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;
# Take a full backup from node1.
my $backup1path = $node1->backup_dir . '/backup1';
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 07da74cf562..2b4f9a48365 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -495,6 +495,10 @@ a directory that's only accessible to the current user to ensure that.
On Windows, we use SSPI authentication to ensure the same (by pg_regress
--config-auth).
+force_initdb => 1 will force to initialized the cluster by initdb. Otherwise, if
+available and if there aren't any parameters, use a previously initdb'd cluster
+as a template by copying it.
+
WAL archiving can be enabled on this node by passing the keyword parameter
has_archiving => 1. This is disabled by default.
@@ -517,6 +521,7 @@ sub init
local %ENV = $self->_get_env();
+ $params{force_initdb} = 0 unless defined $params{force_initdb};
$params{allows_streaming} = 0 unless defined $params{allows_streaming};
$params{has_archiving} = 0 unless defined $params{has_archiving};
@@ -529,14 +534,14 @@ sub init
mkdir $self->backup_dir;
mkdir $self->archive_dir;
- # If available and if there aren't any parameters, use a previously
- # initdb'd cluster as a template by copying it. For a lot of tests, that's
- # substantially cheaper. Do so only if there aren't parameters, it doesn't
- # seem worth figuring out whether they affect compatibility.
+ # For a lot of tests, that's substantially cheaper to copy previously
+ # initdb'd cluster as a template. Do so only if force_initdb => 0, and there
+ # aren't parameters, it doesn't seem worth figuring out whether they affect
+ # compatibility.
#
# There's very similar code in pg_regress.c, but we can't easily
# deduplicate it until we require perl at build time.
- if (defined $params{extra} or !defined $ENV{INITDB_TEMPLATE})
+ if ($params{force_initdb} or defined $params{extra} or !defined $ENV{INITDB_TEMPLATE})
{
note("initializing database system by running initdb");
PostgreSQL::Test::Utils::system_or_bail('initdb', '-D', $pgdata, '-A',
--
2.18.0
From 93d617f3387e5c066541f298e5b71260ebd15103 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 v8 3/4] 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 788487286a400f0378cd417c90da13be43ad1f18 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 15 Feb 2024 14:33:12 +0530
Subject: [PATCH v8 2/4] Code refactor: get_controlfile() to accept full path
of control file
The current version get_controlfile() accepts data directory path, and
computes the full path for the pg_control file, but it would be better
to have a version that accepts the full path of that pg_control file.
---
src/backend/utils/misc/pg_controldata.c | 8 ++++----
src/bin/pg_checksums/pg_checksums.c | 2 +-
src/bin/pg_combinebackup/pg_combinebackup.c | 2 +-
src/bin/pg_controldata/pg_controldata.c | 2 +-
src/bin/pg_ctl/pg_ctl.c | 2 +-
src/common/controldata_utils.c | 19 ++++++++++++++++---
src/include/common/controldata_utils.h | 3 ++-
7 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/src/backend/utils/misc/pg_controldata.c b/src/backend/utils/misc/pg_controldata.c
index 55435dbcf3a..ea8eb3624d9 100644
--- a/src/backend/utils/misc/pg_controldata.c
+++ b/src/backend/utils/misc/pg_controldata.c
@@ -44,7 +44,7 @@ pg_control_system(PG_FUNCTION_ARGS)
/* read the control file */
LWLockAcquire(ControlFileLock, LW_SHARED);
- ControlFile = get_controlfile(DataDir, &crc_ok);
+ ControlFile = get_dir_controlfile(DataDir, &crc_ok);
LWLockRelease(ControlFileLock);
if (!crc_ok)
ereport(ERROR,
@@ -84,7 +84,7 @@ pg_control_checkpoint(PG_FUNCTION_ARGS)
/* Read the control file. */
LWLockAcquire(ControlFileLock, LW_SHARED);
- ControlFile = get_controlfile(DataDir, &crc_ok);
+ ControlFile = get_dir_controlfile(DataDir, &crc_ok);
LWLockRelease(ControlFileLock);
if (!crc_ok)
ereport(ERROR,
@@ -175,7 +175,7 @@ pg_control_recovery(PG_FUNCTION_ARGS)
/* read the control file */
LWLockAcquire(ControlFileLock, LW_SHARED);
- ControlFile = get_controlfile(DataDir, &crc_ok);
+ ControlFile = get_dir_controlfile(DataDir, &crc_ok);
LWLockRelease(ControlFileLock);
if (!crc_ok)
ereport(ERROR,
@@ -216,7 +216,7 @@ pg_control_init(PG_FUNCTION_ARGS)
/* read the control file */
LWLockAcquire(ControlFileLock, LW_SHARED);
- ControlFile = get_controlfile(DataDir, &crc_ok);
+ ControlFile = get_dir_controlfile(DataDir, &crc_ok);
LWLockRelease(ControlFileLock);
if (!crc_ok)
ereport(ERROR,
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 9e6fd435f60..4bdd85f42e0 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -545,7 +545,7 @@ main(int argc, char *argv[])
}
/* Read the control file and check compatibility */
- ControlFile = get_controlfile(DataDir, &crc_ok);
+ ControlFile = get_dir_controlfile(DataDir, &crc_ok);
if (!crc_ok)
pg_fatal("pg_control CRC value is incorrect");
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 31ead7f4058..00c018351ac 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -534,7 +534,7 @@ check_control_files(int n_backups, char **backup_dirs)
controlpath = psprintf("%s/%s", backup_dirs[i], "global/pg_control");
pg_log_debug("reading \"%s\"", controlpath);
- control_file = get_controlfile(backup_dirs[i], &crc_ok);
+ control_file = get_controlfile(controlpath, &crc_ok);
/* Control file contents not meaningful if CRC is bad. */
if (!crc_ok)
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 93e0837947c..1e615131612 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -165,7 +165,7 @@ main(int argc, char *argv[])
}
/* get a copy of the control file */
- ControlFile = get_controlfile(DataDir, &crc_ok);
+ ControlFile = get_dir_controlfile(DataDir, &crc_ok);
if (!crc_ok)
{
pg_log_warning("calculated CRC checksum does not match value stored in control file");
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 6900b27675e..1d887c1b9df 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2174,7 +2174,7 @@ get_control_dbstate(void)
{
DBState ret;
bool crc_ok;
- ControlFileData *control_file_data = get_controlfile(pg_data, &crc_ok);
+ ControlFileData *control_file_data = get_dir_controlfile(pg_data, &crc_ok);
if (!crc_ok)
{
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 92e8fed6b2e..43566920fed 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -49,11 +49,10 @@
* file data is correct.
*/
ControlFileData *
-get_controlfile(const char *DataDir, bool *crc_ok_p)
+get_controlfile(const char *ControlFilePath, bool *crc_ok_p)
{
ControlFileData *ControlFile;
int fd;
- char ControlFilePath[MAXPGPATH];
pg_crc32c crc;
int r;
#ifdef FRONTEND
@@ -64,7 +63,6 @@ get_controlfile(const char *DataDir, bool *crc_ok_p)
Assert(crc_ok_p);
ControlFile = palloc_object(ControlFileData);
- snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
#ifdef FRONTEND
INIT_CRC32C(last_crc);
@@ -162,6 +160,21 @@ retry:
return ControlFile;
}
+/*
+ * get_dir_controlfile()
+ *
+ * Get controlfile values of the given data directory.
+ */
+ControlFileData *
+get_dir_controlfile(const char *DataDir, bool *crc_ok_p)
+{
+ char ControlFilePath[MAXPGPATH];
+
+ snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
+
+ return get_controlfile(ControlFilePath, crc_ok_p);
+}
+
/*
* update_controlfile()
*
diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h
index 04da70e87b2..56528360663 100644
--- a/src/include/common/controldata_utils.h
+++ b/src/include/common/controldata_utils.h
@@ -12,7 +12,8 @@
#include "catalog/pg_control.h"
-extern ControlFileData *get_controlfile(const char *DataDir, bool *crc_ok_p);
+extern ControlFileData *get_controlfile(const char *ControlFilePath, bool *crc_ok_p);
+extern ControlFileData *get_dir_controlfile(const char *DataDir, bool *crc_ok_p);
extern void update_controlfile(const char *DataDir,
ControlFileData *ControlFile, bool do_sync);
--
2.18.0
From da54c83d1a5c11bf978319b3dc6858f2de3e1d4b Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 15 Feb 2024 18:13:29 +0530
Subject: [PATCH v8 4/4] 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 | 82 ++++++++++++++++++-
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, 367 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 e994ee66bbf..46cf2ad5c74 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 86cc01a640b..e4592834852 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -966,4 +966,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 00c018351ac..b5486aad1de 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 5d425209211..d79348ab6f3 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;
@@ -80,6 +81,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..7f730514f3e 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,8 @@ static void report_manifest_error(JsonManifestParseContext *context,
const char *fmt,...)
pg_attribute_printf(2, 3) pg_attribute_noreturn();
+static void verify_system_identifier(manifest_data *manifest,
+ char *controlpath);
static void verify_backup_directory(verifier_context *context,
char *relpath, char *fullpath);
static void verify_backup_file(verifier_context *context,
@@ -375,9 +384,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 +439,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 +470,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 +552,39 @@ 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(manifest_data *manifest, char *controlpath)
+{
+ ControlFileData *control_file;
+ bool crc_ok;
+
+ pg_log_debug("reading \"%s\"", controlpath);
+ control_file = get_controlfile(controlpath, &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 +718,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->manifest, fullpath);
+
/* 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
