Re: allow spread checkpoints when changing checksums online

Hi Tomas,

I tested v4 on master at 5f14f82280: builds clean, test_checksums passes,
and with log_checkpoints the checkpoints come out as expected --
"checkpoint starting: fast force wait" with the default and "force wait"
with fast => false (the latter genuinely spread: a mere 1193 buffers took
write=26.7s, paced to checkpoint_timeout=30s x completion_target 0.9). The
runtime update Daniel caught in v2 also works end-to-end now: starting an
enable with the default and calling pg_enable_data_checksums(..., fast =>
false) mid-run makes the final checkpoint spread.

Two things for v5, since you're updating it anyway:

1) Daniel's terminology comment on v2 seems to have slipped through -- the
new fast paragraphs still say "the time needed to enable checksums" /
"disable checksums" (func-admin.sgml), while the surrounding text
consistently says "data checksums".

2) You asked whether there is a particular scenario behind Fujii's locking
question -- there is one, in exactly the site he quoted. The sequence (line
numbers from datachecksum_state.c with v4 applied):

1. a worker is mid-run, and pg_enable/disable is called again, so
launch_fast_checkpoint gets a new value;
2. the launcher decides to exit and SIGTERMs the worker (launcher_exit(),
:1025) -- but SIGTERM is asynchronous, so the worker keeps running
until its next abort check;
3. before that check, the worker runs its cost_delay/fast_checkpoint
update block (DataChecksumsWorkerMain(), :1727) and stores
fast_checkpoint under DataChecksumsWorkerLock;
4. launcher_exit(), now past the SIGTERM, reads fast_checkpoint UNLOCKED
in SetDataChecksumsOff(DataChecksumState->fast_checkpoint) at :1036.

Steps 3 and 4 race. The other three reads are safe as you argue -- no
worker can be writing the field there: the enable-path reads run only after
the workers have exited and been waited for (ProcessDatabase() ->
WaitForBackgroundWorkerShutdown()), and the disable path spawns no
per-database worker at all. Still harmless for a bool -- either the old
value or the latest requested one, and taking the lock wouldn't make that
any more deterministic -- so this doesn't change the conclusion. But if v5
takes the lock for protocol consistency, note the same unlocked read is in
four places, not one: launcher_exit() (:1036), the two SetDataChecksums*
calls in DataChecksumsWorkerLauncherMain() (:1233, :1245), and the
ProcessAllDatabases() failure path (:1354).

Two more cosmetic bits while you're editing for v5: the commit message has
"perform perform", "ckeckpoints" and "Revided"; and the new sgml paragraphs
say "The <parameter>fast</parameter> specifies ...", which renders as "The
fast specifies" -- "The <parameter>fast</parameter> parameter specifies"
matches how the docs phrase it elsewhere.

Regards,
Rui