Re: Add system identifier to backup manifest

On Wed, Feb 14, 2024 at 12:29:07PM +0530, Amul Sul wrote:
> Ok, I did that way in the attached version, I have passed the control file's
> full path as a second argument to verify_system_identifier() what we gets in
> verify_backup_file(), but that is not doing any useful things with it,
> since we
> were using get_controlfile() to open the control file, which takes the
> directory as an input and computes the full path on its own.

I've read through the patch, and that's pretty cool.

-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)

In 0001, should the comment describing this routine be updated as
well?

+ identifier with pg_control of the backup directory or fails verification

This is missing a <filename> markup here.

+ <productname>PostgreSQL</productname> 17, it is 2; in older versions,
+ it is 1.

Perhaps a couple of <literal>s here.

+ if (strcmp(parse->manifest_version, "1") != 0 &&
+ strcmp(parse->manifest_version, "2") != 0)
+ json_manifest_parse_failure(parse->context,
+ "unexpected manifest version");
+
+ /* Parse version. */
+ version = strtoi64(parse->manifest_version, &ep, 10);
+ if (*ep)
+ json_manifest_parse_failure(parse->context,
+ "manifest version not an integer");
+
+ /* Invoke the callback for version */
+ context->version_cb(context, version);

Shouldn't these two checks be reversed? And is there actually a need
for the first check at all knowing that the version callback should be
in charge of performing the validation vased on the version received?

+my $node2;
+{
+ local $ENV{'INITDB_TEMPLATE'} = undef;

Not sure that it is a good idea to duplicate this pattern twice.
Shouldn't this be something we'd want to control with an option in the
init() method instead?

+static void
+verify_system_identifier(verifier_context *context, char *controlpath)

Relying both on controlpath, being a full path to the control file
including the data directory, and context->backup_directory to read
the contents of the control file looks a bit weird. Wouldn't it be
cleaner to just use one of them?
--
Michael