Re: allow spread checkpoints when changing checksums online - Mailing list pgsql-hackers

On 7/13/26 11:08, Rui Zhao wrote:
> 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".
> 

Yeah, let's be consistent. "data checksums" is better.

> 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).
> 

Good point. I failed to realize launcher_exit() is obviously executed by
the launcher, not the worker. However, should it really be using the
fast_checkpoint flag in launcher_exit()? Shouldn't it be using the
launch_fast_checkpoint instead?

I think that's actually more correct, because that's what the launcher
was requested to use most recently. While fast_checkpoint is what the
worker "knows" about at that point, and maybe it's stale if it didn't
run the update block in DataChecksumsWorkerMain.

Ofc, reading launch_fast_checkpoint still requires a lock, because the
shmem flag is set by some other process when "starting" the launcher.


> 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.
> 

Thanks. You're right there's a bunch of typos, I'll fix them.


regards

-- 
Tomas Vondra