Re: Add system identifier to backup manifest - Mailing list pgsql-hackers

On Mon, Jan 22, 2024 at 2:22 AM Amul Sul <sulamul@gmail.com> wrote:
> Thinking a bit more on this, I realized parse_manifest_file() has many out
> parameters. Instead parse_manifest_file() should simply return manifest data
> like load_backup_manifest().  Attached 0001 patch doing the same, and removed
> parser_context structure, and added manifest_data, and did the required
> adjustments to pg_verifybackup code.

        InitializeBackupManifest(&manifest, opt->manifest,
-
opt->manifest_checksum_type);
+
opt->manifest_checksum_type,
+                                                        GetSystemIdentifier());

InitializeBackupManifest() can just call GetSystemIdentifier() itself,
instead of passing another parameter, I think.

+       if (manifest_version == 1)
+               context->error_cb(context,
+                                                 "%s: backup manifest
doesn't support incremental changes",
+
private_context->backup_directory);

I think this is weird. First, there doesn't seem to be any reason to
bounce through error_cb() here. You could just call pg_fatal(), as we
do elsewhere in this file. Second, there doesn't seem to be any need
to include the backup directory in this error message. We include the
file name (not the directory name) in errors that pertain to the file
itself, like if we can't open or read it. But we don't do that for
semantic errors about the manifest contents (cf.
combinebackup_per_file_cb). This file would need a lot fewer charges
if you didn't feed the backup directory name through here. Third, the
error message is not well-chosen, because a user who looks at it won't
understand WHY the manifest doesn't support incremental changes. I
suggest "backup manifest version 1 does not support incremental
backup".

+       /* Incremental backups supported on manifest version 2 or later */
+       if (manifest_version == 1)
+               context->error_cb(context,
+                                                 "incremental backups
cannot be taken for this backup");

Let's use the same error message as in the previous case here also.

+       for (i = 0; i < n_backups; i++)
+       {
+               if (manifests[i]->system_identifier != system_identifier)
+               {
+                       char       *controlpath;
+
+                       controlpath = psprintf("%s/%s",
prior_backup_dirs[i], "global/pg_control");
+
+                       pg_fatal("manifest is from different database
system: manifest database system identifier is %llu, %s system
identifier is %llu",
+                                        (unsigned long long)
manifests[i]->system_identifier,
+                                        controlpath,
+                                        (unsigned long long)
system_identifier);
+               }
+       }

check_control_files() already verifies that all of the control files
contain the same system identifier as each other, so what we're really
checking here is that the backup manifest in each directory has the
same system identifier as the control file in that same directory. One
problem is that backup manifests are optional here, as per the comment
in load_backup_manifests(), so you need to skip over NULL entries
cleanly to avoid seg faulting if one is missing. I also think the
error message should be changed. How about "%s: manifest system
identifier is %llu, but control file has %llu"?

+               context->error_cb(context,
+                                                 "manifest is from
different database system: manifest database system identifier is
%llu, pg_control database system identifier is %llu",
+                                                 (unsigned long long)
manifest_system_identifier,
+                                                 (unsigned long long)
system_identifier);

And here, while I'm kibitzing, how about "manifest system identifier
is %llu, but this system's identifier is %llu"?

-       qr/could not open directory/,
+       qr/could not open file/,

I don't think that the expected error message here should be changing.
Does it really, with the latest patch version? Why? Can we fix that?

+                       else if (!parse->saw_system_identifier_field &&
+
strcmp(parse->manifest_version, "1") != 0)

I don't think this has any business testing the manifest version.
That's something to sort out at some later stage.