Fix comments on data checksum cost settings

The cost parameters for the data checksums worker can be updated by the
user issuing a repeated enable checksum command, but the comments on the
struct members hadn't been updated to reflect this and were out of date.
Another part of the same comment needed better wording to be readable.

Also wrap the reading of the parameters in a lock, there is no live
bug due to not using a lock but it's still the right thing to do.

Author: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reported-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/2176020b-ecbc-438b-9fc3-9c3593d9e6fc@iki.fi

diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c
index a6fdcf114ec7feaa3e65fa861f2bf20c73a9cfdd..04f1a268845882ed8b0ebdb5ed7b5d5dee217a57 100644 (file)
--- a/src/backend/postmaster/datachecksum_state.c
+++ b/src/backend/postmaster/datachecksum_state.c
@@ -306,15 +306,13 @@ typedef struct DataChecksumsStateStruct
        pid_t           worker_pid;
 
        /*
-        * These fields indicate the target state that the launcher is currently
-        * working towards. They can be different from the corresponding launch_*
+        * These fields indicate the target state that the worker is currently
+        * running with.  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.
+        * the launcher/worker was already running.  The worker will periodically
+        * check if new cost settings have been requested, and if so will copy
+        * them from the launch_* fields and reset cost throttling to match the
+        * new values.
         */
        DataChecksumsWorkerOperation operation;
        int                     cost_delay;
@@ -1509,6 +1507,7 @@ DataChecksumsWorkerMain(Datum arg)
        BufferAccessStrategy strategy;
        bool            aborted = false;
        int64           rels_done;
+       bool            process_shared;
 #ifdef USE_INJECTION_POINTS
        bool            retried = false;
 #endif
@@ -1532,10 +1531,13 @@ DataChecksumsWorkerMain(Datum arg)
 
        /*
         * Get a list of all temp tables present as we start in this database. We
-        * need to wait until they are all gone until we are done, since we cannot
-        * access these relations and modify them.
+        * need to wait until they are all gone before we exit.  For the list of
+        * relations to enable checksums in, check if shared catalogs have been
+        * processed already.
         */
        InitialTempTableList = BuildRelationList(true, false);
+       LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
+       process_shared = DataChecksumState->process_shared_catalogs;
 
        /*
         * Enable vacuum cost delay, if any.  While this process isn't doing any
@@ -1549,6 +1551,7 @@ DataChecksumsWorkerMain(Datum arg)
         */
        VacuumCostDelay = DataChecksumState->cost_delay;
        VacuumCostLimit = DataChecksumState->cost_limit;
+       LWLockRelease(DataChecksumsWorkerLock);
        VacuumUpdateCosts();
        VacuumCostBalance = 0;
 
@@ -1557,8 +1560,7 @@ DataChecksumsWorkerMain(Datum arg)
         */
        strategy = GetAccessStrategy(BAS_VACUUM);
 
-       RelationList = BuildRelationList(false,
-                                                                        DataChecksumState->process_shared_catalogs);
+       RelationList = BuildRelationList(false, process_shared);
 
        /* Update the total number of relations to be processed in this DB. */
        {