Re: Offline enabling/disabling of data checksums

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.