> On 17 Jun 2026, at 09:42, Heikki Linnakangas <[email protected]> wrote: > > Hi, > > The comment in the DataChecksumsStateStruct says: > >> /* >> * These fields indicate the target state that the launcher is currently >> * working towards. They can be different from the corresponding launch_* >> * fields, if a new pg_enable/disable_data_checksums() call was made while >> * the launcher/worker was already running. >> * >> * The below members are set when the launcher starts, and are only >> * accessed read-only by the single worker. Thus, we can access these >> * without a lock. If multiple workers, or dynamic cost parameters, are >> * supported at some point then this would need to be revisited. >> */ >> DataChecksumsWorkerOperation operation; >> int cost_delay; >> int cost_limit; > > The "only accessed read-only by the single worker" part isn't true, because > DataChecksumsWorkerMain() does this: > >> /* >> * Check if the cost settings changed during runtime and if so, update >> * to reflect the new values and signal that the access strategy needs >> * to be refreshed. >> */ >> LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE); >> if ((DataChecksumState->launch_cost_delay != DataChecksumState->cost_delay) >> || (DataChecksumState->launch_cost_limit != DataChecksumState->cost_limit)) >> { >> costs_updated = true; >> VacuumCostDelay = DataChecksumState->launch_cost_delay; >> VacuumCostLimit = DataChecksumState->launch_cost_limit; >> VacuumUpdateCosts(); >> DataChecksumState->cost_delay = DataChecksumState->launch_cost_delay; >> DataChecksumState->cost_limit = DataChecksumState->launch_cost_limit; >> } >> else >> costs_updated = false; >> LWLockRelease(DataChecksumsWorkerLock); > > So contrary to the comment, the cost_delay fields are updated by the worker.
Ugh, that comment was missed when I added the ability to update the cost params during runtime. > Some other observations: > > - at the beginning of DataChecksumsWorkerMain(), it reads > DataChecksumState->cost_delay and cost_limit without holding the lock, and > also DataChecksumState->process_shared_catalogs. I think that's OK, but I > would just add locking to it, to remove all doubt. Agreed, it should be wrapped in a lock even if it's (currently) safe. > - the comment says "These fields indicate the target state that the launcher > is currently working towards". It took me a moment to understand what the > means. At first, I thought that it means that it's the target state of the > launcher, but not necessarily what it's currently using. That's wrong. Those > are in fact the values that the *worker* is *currently* using. > launch_cost_delay/limit are the "target" values, which will eventually be > copied into the current cost_delay/limit values. Perhaps change "... launcher > is currently working towards" into "... worker is currently running with" or > something like that. Yes, that sentence was clearly a pretty strong wordsoup. Thanks for pointing it out. All comments addressed in the attached. -- Daniel Gustafsson
0001-Fix-comments-on-data-checksum-cost-settings.patch
Description: Binary data
