Hi Tomas, Thanks, the new patch addresses the earlier points -- builds clean, test_checksums passes, disabling with fast => false still spreads. Switching launcher_exit() to launch_fast_checkpoint reads right: it's the source of truth, set by the user's pg_enable/disable call, while fast_checkpoint is the effective copy the worker maintains -- so reading launch_fast_checkpoint from the launcher gets the current intent and can't race a worker write the way reading fast_checkpoint could.
On the locking: three of the four reads take the lock -- launcher_exit() (:1044) and the two in DataChecksumsWorkerLauncherMain() (:1248, :1275) -- but ProcessAllDatabases() (:1388) still reads DataChecksumState->fast_checkpoint inline, unlocked. It's safe there (no worker is running at that point), so :1388 is just the read that slipped past the locking protocol; and where the lock is taken it's LW_EXCLUSIVE on plain reads, where LW_SHARED would do (the writers already hold it exclusive), :1388 included once locked. More substantive: now that launcher_exit() reads launch_fast_checkpoint, the three still reading fast_checkpoint (:1248, :1275, :1388) look inconsistent -- should they move to launch_fast_checkpoint too? One broader observation, and it predates the patch -- from the original online-checksums commit; this one just adds fast_checkpoint to the pattern. Each of the four non-launch_ "effective" fields mirrors a launch_ field, yet each has at most one real reader: operation is never read (write-only at :1187/:1303), cost_delay/cost_limit only by the worker (:1691, plus the change-check at :1750), and fast_checkpoint only by the launcher for its checkpoint (the worker's :1752 compare just keeps the mirror in step). The worker's use of the cost fields is a "what did I last apply" baseline that could as easily be process-local, and in each case the launch_ value is right there to read directly. So the whole effective group looks like it may not need to be shared state at all -- derivable from the launch_ fields plus a worker-local baseline. Nothing to settle in this patch, and maybe there's intent behind the mirror I'm missing, but since the patch grows the group by one it seemed worth putting on the table. One doc nit, since 0001 is backpatched: typos in the added text -- "generate of lot of writes" -> "a lot", "requresting spread checkpoints" -> "requesting", and the I/O bullet's "enabling and disabling data checksums requires performing checkpoints" -> "require". On the "checksums" vs "data checksums" wording in wal.sgml: that's the whole section's existing style, not something this patch touches, so I'd leave it out of a backpatched doc fix -- a separate cleanup if anyone cares. And +1 on the retitle to "Impact of Online Operations on System". Regards, Rui
