Improve comments in online checksums code

Spelling fixes and rewording outdated information.

Author: Tomas Vondra <tomas@vondra.me>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/f1281cf3-89a3-4936-9bc5-2a5a6291229f@vondra.me

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b32f54f8402696d353b01308e8d63a0708f5383f..d69d03b2ef368997975681e78818b0a629ea2b4c 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4807,9 +4807,8 @@ SetDataChecksumsOn(void)
 
        /*
         * The only allowed state transition to "on" is from "inprogress-on" since
-        * that state ensures that all pages will have data checksums written.  No
-        * such state transition exists, if it does happen it's likely due to a
-        * programmer error.
+        * that state ensures that all pages will have data checksums written. Any
+        * other attempted state transition is likely due to a programmer error.
         */
        if (XLogCtl->data_checksum_version != PG_DATA_CHECKSUM_INPROGRESS_ON)
        {

diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 9803a0ee2a14112e7f7f38e81c1a0395ddc7172a..ad4bf4bd2a8e90ec5c62725e0d543aaf613f0387 100644 (file)
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -88,7 +88,7 @@ AuxiliaryProcessMainCommon(void)
         *
         * The postmaster (which is what gets forked into the new child process)
         * does not handle barriers, therefore it may not have the current value
-        * of LocalDataChecksumVersion value (it'll have the value read from the
+        * of LocalDataChecksumState value (it'll have the value read from the
         * control file, which may be arbitrarily old).
         *
         * NB: Even if the postmaster handled barriers, the value might still be

diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c
index 33430147ff29378499cd9c653e6aa95a78b5ad90..b61674ae95736a4c71d772faa2057e43e4d6c86d 100644 (file)
--- a/src/backend/postmaster/datachecksum_state.c
+++ b/src/backend/postmaster/datachecksum_state.c
@@ -20,8 +20,8 @@
  * When enabling checksums in an online cluster, data_checksums will be set to
  * "inprogress-on" which signals that write operations MUST compute and write
  * the checksum on the data page, but during reading the checksum SHALL NOT be
- * verified. This ensures that all objects created during when checksums are
- * being enabled will have checksums set, but reads won't fail due to missing or
+ * verified. This ensures that all objects created while checksums are being
+ * enabled will have checksums set, but reads won't fail due to missing or
  * invalid checksums. Invalid checksums can be present in case the cluster had
  * checksums enabled, then disabled them and updated the page while they were
  * disabled.
@@ -299,8 +299,9 @@ typedef struct DataChecksumsStateStruct
        bool            launcher_running;
 
        /*
-        * Is a worker process currently running?  This is set by the worker
-        * launcher when it starts waiting for a worker process to finish.
+        * PID of the worker process, if it's currently running, of InvalidPid if
+        * none. This is set by the worker launcher when it starts waiting for a
+        * worker process to finish.
         */
        int                     worker_pid;
 
@@ -320,12 +321,8 @@ typedef struct DataChecksumsStateStruct
        int                     cost_limit;
 
        /*
-        * Signaling between the launcher and the worker process.
-        *
-        * As there is only a single worker, and the launcher won't read these
-        * until the worker exits, they can be accessed without the need for a
-        * lock. If multiple workers are supported then this will have to be
-        * revisited.
+        * Signaling between the launcher and the worker process. Protected by
+        * DataChecksumsWorkerLock.
         */
 
        /* result, set by worker before exiting */
@@ -599,9 +596,9 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
         *
         * If the launcher is currently busy enabling the checksums, and we want
         * them disabled (or vice versa), the launcher will notice that at latest
-        * when it's about to exit, and will loop back process the new request. So
-        * if the launcher is already running, we don't need to do anything more
-        * here to abort it.
+        * when it's about to exit, and will loop back to process the new request.
+        * So if the launcher is already running, we don't need to do anything
+        * more here to abort it.
         *
         * If you call pg_enable/disable_data_checksums() twice in a row, before
         * the launcher has had a chance to start up, we still end up launching it
@@ -710,10 +707,7 @@ ProcessSingleRelationFork(Relation reln, ForkNumber forkNum, BufferAccessStrateg
 
                UnlockReleaseBuffer(buf);
 
-               /*
-                * This is the only place where we check if we are asked to abort, the
-                * abortion will bubble up from here.
-                */
+               /* Check if we are asked to abort, the abortion will bubble up. */
                Assert(operation == ENABLE_DATACHECKSUMS);
                LWLockAcquire(DataChecksumsWorkerLock, LW_SHARED);
                if (DataChecksumState->launch_operation == DISABLE_DATACHECKSUMS)
@@ -924,8 +918,8 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db)
  * performed checksum operations exits. A launcher process which is exiting due
  * to a duplicate started launcher does not need to perform any cleanup and
  * this function should not be called. Otherwise, we need to clean up the abort
- * flag to ensure that processing started again if it was previously aborted
- * (note: started again, *not* restarted from where it left off).
+ * flag to ensure that processing can be started again if it was previously
+ * aborted (note: started again, *not* restarted from where it left off).
  */
 static void
 launcher_exit(int code, Datum arg)
@@ -1434,7 +1428,7 @@ FreeDatabaseList(List *dblist)
  * BuildRelationList
  *             Compile a list of relations in the database
  *
- * Returns a list of OIDs for the request relation types. If temp_relations
+ * Returns a list of OIDs for the requested relation types. If temp_relations
  * is True then only temporary relations are returned. If temp_relations is
  * False then non-temporary relations which have data checksums are returned.
  * If include_shared is True then shared relations are included as well in a

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2460e550f96e286b32aa37bcb98450c732d13cf0..c1457eb34f02b28dd7ee781b2f5e2a7f917f972c 100644 (file)
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -773,7 +773,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
         *
         * The postmaster (which is what gets forked into the new child process)
         * does not handle barriers, therefore it may not have the current value
-        * of LocalDataChecksumVersion value (it'll have the value read from the
+        * of LocalDataChecksumState value (it'll have the value read from the
         * control file, which may be arbitrarily old).
         *
         * NB: Even if the postmaster handled barriers, the value might still be