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