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.
I plan to commit this, barring objections.
--
Robert Haas
EDB: http://www.enterprisedb.com