Re: Offline enabling/disabling of data checksums

Bonjour Michaël,

> Please find attached an updated patch set, I have rebased that stuff
> on top of my recent commits to refactor the control file updates.

Patch applies cleanly, compiles, make check-world seems ok, doc build ok.

It would help if the patch includes a version number. I assume that this 
is v7.

Doc looks ok.

Moving the controlfile looks like an effective way to prevent any 
concurrent start, as the fs operation is probably atomic and especially if 
external tools uses the same trick. However this is not the case yet, eg 
"pg_resetwal" uses a "postmaster.pid" hack instead. Probably the method 
could be unified, possibly with some functions in "controlfile_utils.c".

However, I think that there still is a race condition because of the order 
in which it is implemented:

  pg_checksums reads control file
  pg_checksums checks control file contents...
  ** cluster may be started and the control file updated
  pg_checksums moves the (updated) control file
  pg_checksums proceeds on a running cluster
  pg_checksums moves back the control file
  pg_checksums updates the control file contents, overriding updates

I think that the correct way to handle this for enable/disable is:

  pg_checksums moves the control file
  pg_checksums reads, checks, proceeds, updates
  pg_checksums moves back the control file

This probably means extending a little bit the update_controlfile function 
to allow a suffix. No big deal.

Ok, this might not work, because of the following, less likely, race 
condition:

  postmaster opens control file RW
  pg_checksums moves control file, posmater open file handle follows
  ...

So ISTM that we really need some locking to have something clean.

Why not always use "pgrename" instead of the strange pg_mv_file macro?

Help line about --check not simplified as suggested in a prior review, 
although you said you would take it into account.

Tests look ok.

-- 
Fabien.