Skip to content

Commit 2041bc4

Browse files
robertmhaasrobertmhaas
committed
Add the system identifier to backup manifests.
Before this patch, if you took a full backup on server A and then tried to use the backup manifest to take an incremental backup on server B, it wouldn't know that the manifest was from a different server and so the incremental backup operation could potentially complete without error. When you later tried to run pg_combinebackup, you'd find out that your incremental backup was and always had been invalid. That's poor timing, because nobody likes finding out about backup problems only at restore time. With this patch, you'll get an error when trying to take the (invalid) incremental backup, which seems a lot nicer. Amul Sul, revised by me. Review by Michael Paquier. Discussion: http://postgr.es/m/CA+TgmoYLZzbSAMM3cAjV4Y+iCRZn-bR9H2+Mdz7NdaJFU1Zb5w@mail.gmail.com
1 parent 1ee910c commit 2041bc4

16 files changed

+351
-23
lines changed

‎doc/src/sgml/backup-manifest.sgml

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,22 @@
3737
<term><literal>PostgreSQL-Backup-Manifest-Version</literal></term>
3838
<listitem>
3939
<para>
40-
The associated value is always the integer 1.
40+
The associated value is an integer. Beginning in
41+
<productname>PostgreSQL</productname> <literal>17</literal>,
42+
it is <literal>2</literal>; in older versions, it is <literal>1</literal>.
43+
</para>
44+
</listitem>
45+
</varlistentry>
46+
47+
<varlistentry>
48+
<term><literal>System-Identifier</literal></term>
49+
<listitem>
50+
<para>
51+
The database system identifier of the
52+
<productname>PostgreSQL</productname> instance where the backup was
53+
taken. This field is present only when
54+
<literal>PostgreSQL-Backup-Manifest-Version</literal> is
55+
<literal>2</literal>.
4156
</para>
4257
</listitem>
4358
</varlistentry>

‎doc/src/sgml/ref/pg_verifybackup.sgml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,10 @@ PostgreSQL documentation
5353
Backup verification proceeds in four stages. First,
5454
<literal>pg_verifybackup</literal> reads the
5555
<literal>backup_manifest</literal> file. If that file
56-
does not exist, cannot be read, is malformed, or fails verification
57-
against its own internal checksum, <literal>pg_verifybackup</literal>
58-
will terminate with a fatal error.
56+
does not exist, cannot be read, is malformed, fails to match the system
57+
identifier with <filename>pg_control</filename> of the backup directory or
58+
fails verification against its own internal checksum,
59+
<literal>pg_verifybackup</literal> will terminate with a fatal error.
5960
</para>
6061

6162
<para>

‎src/backend/backup/backup_manifest.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "postgres.h"
1414

1515
#include "access/timeline.h"
16+
#include "access/xlog.h"
1617
#include "backup/backup_manifest.h"
1718
#include "backup/basebackup_sink.h"
1819
#include "mb/pg_wchar.h"
@@ -77,8 +78,10 @@ InitializeBackupManifest(backup_manifest_info *manifest,
7778

7879
if (want_manifest != MANIFEST_OPTION_NO)
7980
AppendToManifest(manifest,
80-
"{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n"
81-
"\"Files\": [");
81+
"{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n"
82+
"\"System-Identifier\": " UINT64_FORMAT ",\n"
83+
"\"Files\": [",
84+
GetSystemIdentifier());
8285
}
8386

8487
/*

‎src/backend/backup/basebackup_incremental.c

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "postgres.h"
2121

2222
#include "access/timeline.h"
23+
#include "access/xlog.h"
2324
#include "backup/basebackup_incremental.h"
2425
#include "backup/walsummary.h"
2526
#include "common/blkreftable.h"
@@ -113,6 +114,10 @@ struct IncrementalBackupInfo
113114
BlockRefTable *brtab;
114115
};
115116

117+
static void manifest_process_version(JsonManifestParseContext *context,
118+
int manifest_version);
119+
static void manifest_process_system_identifier(JsonManifestParseContext *context,
120+
uint64 manifest_system_identifier);
116121
static void manifest_process_file(JsonManifestParseContext *context,
117122
char *pathname,
118123
size_t size,
@@ -199,6 +204,8 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib)
199204

200205
/* Parse the manifest. */
201206
context.private_data = ib;
207+
context.version_cb = manifest_process_version;
208+
context.system_identifier_cb = manifest_process_system_identifier;
202209
context.per_file_cb = manifest_process_file;
203210
context.per_wal_range_cb = manifest_process_wal_range;
204211
context.error_cb = manifest_report_error;
@@ -927,6 +934,39 @@ hash_string_pointer(const char *s)
927934
return hash_bytes(ss, strlen(s));
928935
}
929936

937+
/*
938+
* This callback to validate the manifest version for incremental backup.
939+
*/
940+
static void
941+
manifest_process_version(JsonManifestParseContext *context,
942+
int manifest_version)
943+
{
944+
/* Incremental backups don't work with manifest version 1 */
945+
if (manifest_version == 1)
946+
context->error_cb(context,
947+
"backup manifest version 1 does not support incremental backup");
948+
}
949+
950+
/*
951+
* This callback to validate the manifest system identifier against the current
952+
* database server.
953+
*/
954+
static void
955+
manifest_process_system_identifier(JsonManifestParseContext *context,
956+
uint64 manifest_system_identifier)
957+
{
958+
uint64 system_identifier;
959+
960+
/* Get system identifier of current system */
961+
system_identifier = GetSystemIdentifier();
962+
963+
if (manifest_system_identifier != system_identifier)
964+
context->error_cb(context,
965+
"manifest system identifier is %llu, but database system identifier is %llu",
966+
(unsigned long long) manifest_system_identifier,
967+
(unsigned long long) system_identifier);
968+
}
969+
930970
/*
931971
* This callback is invoked for each file mentioned in the backup manifest.
932972
*

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -965,4 +965,20 @@
965965
my @dst_tblspc = glob "$backupdir/pg_tblspc/$tblspc_oid/PG_*";
966966
is(@dst_tblspc, 1, 'tblspc directory copied');
967967

968+
# Can't take backup with referring manifest of different cluster
969+
#
970+
# Set up another new database instance with force initdb option. We don't want
971+
# to initializing database system by copying initdb template for this, because
972+
# we want it to be a separate cluster with a different system ID.
973+
my $node2 = PostgreSQL::Test::Cluster->new('node2');
974+
$node2->init(force_initdb => 1, has_archiving => 1, allows_streaming => 1);
975+
$node2->append_conf('postgresql.conf', 'summarize_wal = on');
976+
$node2->start;
977+
978+
$node2->command_fails_like(
979+
[ @pg_basebackup_defs, '-D', "$tempdir" . '/diff_sysid',
980+
'--incremental', "$backupdir" . '/backup_manifest' ],
981+
qr/manifest system identifier is .*, but database system identifier is/,
982+
"pg_basebackup fails with different database system manifest");
983+
968984
done_testing();

‎src/bin/pg_combinebackup/load_manifest.c

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,10 @@ static uint32 hash_string_pointer(char *s);
5050
#define SH_DEFINE
5151
#include "lib/simplehash.h"
5252

53+
static void combinebackup_version_cb(JsonManifestParseContext *context,
54+
int manifest_version);
55+
static void combinebackup_system_identifier_cb(JsonManifestParseContext *context,
56+
uint64 manifest_system_identifier);
5357
static void combinebackup_per_file_cb(JsonManifestParseContext *context,
5458
char *pathname, size_t size,
5559
pg_checksum_type checksum_type,
@@ -153,6 +157,8 @@ load_backup_manifest(char *backup_directory)
153157
result = pg_malloc0(sizeof(manifest_data));
154158
result->files = ht;
155159
context.private_data = result;
160+
context.version_cb = combinebackup_version_cb;
161+
context.system_identifier_cb = combinebackup_system_identifier_cb;
156162
context.per_file_cb = combinebackup_per_file_cb;
157163
context.per_wal_range_cb = combinebackup_per_wal_range_cb;
158164
context.error_cb = report_manifest_error;
@@ -181,6 +187,31 @@ report_manifest_error(JsonManifestParseContext *context, const char *fmt,...)
181187
exit(1);
182188
}
183189

190+
/*
191+
* This callback to validate the manifest version number for incremental backup.
192+
*/
193+
static void
194+
combinebackup_version_cb(JsonManifestParseContext *context,
195+
int manifest_version)
196+
{
197+
/* Incremental backups supported on manifest version 2 or later */
198+
if (manifest_version == 1)
199+
pg_fatal("backup manifest version 1 does not support incremental backup");
200+
}
201+
202+
/*
203+
* Record system identifier extracted from the backup manifest.
204+
*/
205+
static void
206+
combinebackup_system_identifier_cb(JsonManifestParseContext *context,
207+
uint64 manifest_system_identifier)
208+
{
209+
manifest_data *manifest = context->private_data;
210+
211+
/* Validation will be at the later stage */
212+
manifest->system_identifier = manifest_system_identifier;
213+
}
214+
184215
/*
185216
* Record details extracted from the backup manifest for one file.
186217
*/

‎src/bin/pg_combinebackup/load_manifest.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ typedef struct manifest_wal_range
5555
*/
5656
typedef struct manifest_data
5757
{
58+
uint64 system_identifier;
5859
manifest_files_hash *files;
5960
manifest_wal_range *first_wal_range;
6061
manifest_wal_range *last_wal_range;

‎src/bin/pg_combinebackup/pg_combinebackup.c

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ cb_cleanup_dir *cleanup_dir_list = NULL;
9292

9393
static void add_tablespace_mapping(cb_options *opt, char *arg);
9494
static StringInfo check_backup_label_files(int n_backups, char **backup_dirs);
95-
static void check_control_files(int n_backups, char **backup_dirs);
95+
static uint64 check_control_files(int n_backups, char **backup_dirs);
9696
static void check_input_dir_permissions(char *dir);
9797
static void cleanup_directories_atexit(void);
9898
static void create_output_directory(char *dirname, cb_options *opt);
@@ -134,11 +134,13 @@ main(int argc, char *argv[])
134134

135135
const char *progname;
136136
char *last_input_dir;
137+
int i;
137138
int optindex;
138139
int c;
139140
int n_backups;
140141
int n_prior_backups;
141142
int version;
143+
uint64 system_identifier;
142144
char **prior_backup_dirs;
143145
cb_options opt;
144146
cb_tablespace *tablespaces;
@@ -216,7 +218,7 @@ main(int argc, char *argv[])
216218

217219
/* Sanity-check control files. */
218220
n_backups = argc - optind;
219-
check_control_files(n_backups, argv + optind);
221+
system_identifier = check_control_files(n_backups, argv + optind);
220222

221223
/* Sanity-check backup_label files, and get the contents of the last one. */
222224
last_backup_label = check_backup_label_files(n_backups, argv + optind);
@@ -231,6 +233,26 @@ main(int argc, char *argv[])
231233
/* Load backup manifests. */
232234
manifests = load_backup_manifests(n_backups, prior_backup_dirs);
233235

236+
/*
237+
* Validate the manifest system identifier against the backup system
238+
* identifier.
239+
*/
240+
for (i = 0; i < n_backups; i++)
241+
{
242+
if (manifests[i] &&
243+
manifests[i]->system_identifier != system_identifier)
244+
{
245+
char *controlpath;
246+
247+
controlpath = psprintf("%s/%s", prior_backup_dirs[i], "global/pg_control");
248+
249+
pg_fatal("%s: manifest system identifier is %llu, but control file has %llu",
250+
controlpath,
251+
(unsigned long long) manifests[i]->system_identifier,
252+
(unsigned long long) system_identifier);
253+
}
254+
}
255+
234256
/* Figure out which tablespaces are going to be included in the output. */
235257
last_input_dir = argv[argc - 1];
236258
check_input_dir_permissions(last_input_dir);
@@ -256,7 +278,7 @@ main(int argc, char *argv[])
256278
/* If we need to write a backup_manifest, prepare to do so. */
257279
if (!opt.dry_run && !opt.no_manifest)
258280
{
259-
mwriter = create_manifest_writer(opt.output);
281+
mwriter = create_manifest_writer(opt.output, system_identifier);
260282

261283
/*
262284
* 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)
517539
}
518540

519541
/*
520-
* Sanity check control files.
542+
* Sanity check control files and return system_identifier.
521543
*/
522-
static void
544+
static uint64
523545
check_control_files(int n_backups, char **backup_dirs)
524546
{
525547
int i;
@@ -564,6 +586,8 @@ check_control_files(int n_backups, char **backup_dirs)
564586
*/
565587
pg_log_debug("system identifier is %llu",
566588
(unsigned long long) system_identifier);
589+
590+
return system_identifier;
567591
}
568592

569593
/*

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
use warnings FATAL => 'all';
99
use File::Compare;
1010
use File::Path qw(rmtree);
11+
use File::Copy;
1112
use PostgreSQL::Test::Cluster;
1213
use PostgreSQL::Test::Utils;
1314
use Test::More;
@@ -79,6 +80,19 @@
7980
qr/expected system identifier.*but found/,
8081
"can't combine backups from different nodes");
8182

83+
# Can't combine when different manifest system identifier
84+
rename("$backup2path/backup_manifest", "$backup2path/backup_manifest.orig")
85+
or die "could not move $backup2path/backup_manifest";
86+
copy("$backupother2path/backup_manifest", "$backup2path/backup_manifest")
87+
or die "could not copy $backupother2path/backup_manifest";
88+
$node1->command_fails_like(
89+
[ 'pg_combinebackup', $backup1path, $backup2path, $backup3path, '-o', $resultpath ],
90+
qr/ manifest system identifier is .*, but control file has /,
91+
"can't combine backups with different manifest system identifier ");
92+
# Restore the backup state
93+
move("$backup2path/backup_manifest.orig", "$backup2path/backup_manifest")
94+
or die "could not move $backup2path/backup_manifest";
95+
8296
# Can't omit a required backup.
8397
$node1->command_fails_like(
8498
[ 'pg_combinebackup', $backup1path, $backup3path, '-o', $resultpath ],

‎src/bin/pg_combinebackup/write_manifest.c

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ static size_t hex_encode(const uint8 *src, size_t len, char *dst);
4545
* in the specified directory.
4646
*/
4747
manifest_writer *
48-
create_manifest_writer(char *directory)
48+
create_manifest_writer(char *directory, uint64 system_identifier)
4949
{
5050
manifest_writer *mwriter = pg_malloc(sizeof(manifest_writer));
5151

@@ -57,8 +57,10 @@ create_manifest_writer(char *directory)
5757
pg_checksum_init(&mwriter->manifest_ctx, CHECKSUM_TYPE_SHA256);
5858

5959
appendStringInfo(&mwriter->buf,
60-
"{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n"
61-
"\"Files\": [");
60+
"{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n"
61+
"\"System-Identifier\": " UINT64_FORMAT ",\n"
62+
"\"Files\": [",
63+
system_identifier);
6264

6365
return mwriter;
6466
}

‎src/bin/pg_combinebackup/write_manifest.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ struct manifest_wal_range;
1919
struct manifest_writer;
2020
typedef struct manifest_writer manifest_writer;
2121

22-
extern manifest_writer *create_manifest_writer(char *directory);
22+
extern manifest_writer *create_manifest_writer(char *directory,
23+
uint64 system_identifier);
2324
extern void add_file_to_manifest(manifest_writer *mwriter,
2425
const char *manifest_path,
2526
size_t size, time_t mtime,

0 commit comments

Comments
 (0)