From 19d6bc95cf6b8becaa75dbca9b0fdf1d6ce2aef1 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 28 May 2026 12:02:06 +0200 Subject: [PATCH 2/3] Improve comments in online checksums code Spelling fixes and rewording outdated information. Author: Tomas Vondra Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/f1281cf3-89a3-4936-9bc5-2a5a6291229f@vondra.me --- src/backend/access/transam/xlog.c | 5 ++- src/backend/postmaster/auxprocess.c | 2 +- src/backend/postmaster/datachecksum_state.c | 34 +++++++++------------ src/backend/utils/init/postinit.c | 2 +- 4 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index b32f54f8402..d69d03b2ef3 100644 --- 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 9803a0ee2a1..ad4bf4bd2a8 100644 --- 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 33430147ff2..b61674ae957 100644 --- 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 2460e550f96..c1457eb34f0 100644 --- 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 -- 2.39.3 (Apple Git-146)