On Thu, Mar 7, 2024 at 9:16 AM Robert Haas <robertmhaas@gmail.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.
