Bonjour Michaël,
Here is a partial review:
> - 0001 if a patch to refactor the routine for the control file
> update. I have made it backend-aware, and we ought to be careful with
> error handling, use of fds and such, something that v4 was not very
> careful about.
This refactoring patch is ok for me: applies, compiles, check is ok.
However, Am I right in thinking that the change should propagate to other
tools which manipulate the control file, eg pg_resetwal, postmaster… So
that there would be only one shared API to update the control file?
> - 0002 renames pg_verify_checksums to pg_checksums with a
> straight-forward switch. Docs as well as all references to
> pg_verify_checksums are updated.
Looks ok to me. Applies, compiles, checks are ok. Doc build is ok.
I'm wondering whether there should be something done so that the
inter-release documentation navigation works? Should the section keep the
former name? Is it managed by hand somewhere else? Maybe it would require
to keep the refsect1 id, or to duplicate it, not sure.
In "doc/src/sgml/ref/allfiles.sgml" there seems to be a habit to align on
the SYSTEM keyword, which is not fellowed by the patch.
> - 0003 adds the new options --check, --enable and --disable, with
> --check being the default as discussed.
Looks like the patch I already reviewed, but I'll look at it in details
later.
"If enabling or disabling checksums, the exit status is nonzero if the
operation failed."
However:
+ if (ControlFile->data_checksum_version == 0 &&
+ action == PG_ACTION_DISABLE)
+ {
+ fprintf(stderr, _("%s: data checksums are already disabled in cluster.\n"), progname);
+ exit(1);
+ }
This seem contradictory to me: you want to disable checksum, and they are
already disabled, so nothing is needed. How does that qualifies as a
"failed" operation?
Further review will come later.
> - 0004 adds a -N/--no-sync which I think is nice for consistency with
> other tools. That's also useful for the tests, and was not discussed
> until now on this thread.
Indeed. I do not immediately see the use case where no syncing would be a
good idea. I can see why it would be a bad idea. So I'm not sure of the
concept.
--
Fabien.
