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.