Re: Add system identifier to backup manifest

On Thu, Feb 15, 2024 at 7:18 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

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

Thank you for looking into this.

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

Ok, updated in the attached version.

>
> + identifier with pg_control of the backup directory or fails
> verification
>
> This is missing a <filename> markup here.
>

Done, in the attached version.

>
> + <productname>PostgreSQL</productname> 17, it is 2; in older
> versions,
> + it is 1.
>
> Perhaps a couple of <literal>s here.
>
Done.

> + 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?
>

Make sense, reversed the order.

I think, particular allowed versions should be placed at the central place,
and
the callback can check and react on the versions suitable to them, IMHO.

> +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?
>

Yes, I did that in a separate patch, see 0001 patch.

> +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?
>

Well, yes, I had to have the same feeling, how about having another function
that can accept a full path of pg_control?

I tried in the 0002 patch, where the original function is renamed to
get_dir_controlfile(), which accepts the data directory path as before, and
get_controlfile() now accepts the full path of the pg_control file.

Kindly have a look at the attached version.

Regards,
Amul