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