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

From Michael Paquier
Subject Re: Add system identifier to backup manifest
Date
Msg-id Zc1tX9lLonLGu6oH@paquier.xyz
Whole thread Raw
In response to Re: Add system identifier to backup manifest  (Amul Sul <sulamul@gmail.com>)
Responses Re: Add system identifier to backup manifest
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Peter Geoghegan
Date:
Subject: Re: index prefetching
Next
From: Michael Paquier
Date:
Subject: Re: Can we include capturing logs of pgdata/pg_upgrade_output.d/*/log in buildfarm