Re: Add system identifier to backup manifest

On Fri, Mar 8, 2024 at 1:22 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Mar 7, 2024 at 9:16 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > It could. I just thought this was clearer. I agree that it's a bit
> > long, but I don't think this is worth bikeshedding very much. If at a
> > later time somebody feels strongly that it needs to be changed, so be
> > it. Right now, getting on with the business at hand is more important,
> > IMHO.
>
> Here's a new version of the patch set, rebased over my version of 0001
> and with various other corrections:
>
> * Tidy up grammar in documentation.
> * In manifest_process_version, the test checked whether the manifest
> version == 1, but the comment talked about versions >= 2. Make the
> comment match the code.
> * In load_backup_manifest, avoid changing the existing placement of a
> variable declaration.
> * Rename verify_system_identifier to verify_control_file because if we
> were verifying multiple things about the control file we'd still want
> to only read it one.
> * Tweak the coding of verify_backup_file and verify_control_file to
> avoid repeated path construction.
> * Remove saw_system_identifier_field. This looks like it's trying to
> enforce a rule that the system identifier must immediately follow the
> version, but we don't insist on anything like that for files or wal
> ranges, so there seems to be no reason to do it here.
> * Remove bogus "unrecognized top-level field" test from
> 005_bad_manifest.pl. The JSON included here doesn't include any
> unrecognized top-level field, so the fact that we were getting that
> error message was wrong. After removing saw_system_identifier_field,
> we no longer get the wrong error message any more, so the test started
> failing.
> * Remove "expected system identifier" test from 005_bad_manifest.pl.
> This was basically a test that saw_system_identifier_field was
> working.
> * Header comment adjustment for
> json_manifest_finalize_system_identifier. The last sentence was
> cut-and-pasted from somewhere that it made sense to here, where it
> doesn't. There's only ever one system identifier.
>
>
Thank you for the improvement.

The caller of verify_control_file() has the full path of the control file
that
can pass it and avoid recomputing. With this change, it doesn't really need
verifier_context argument -- only the manifest's system identifier is enough
along with the control file path. Did the same in the attached delta patch
for v11-0002 patch, please have a look, thanks.

Regards,
Amul