Re: Changing the state of data checksums in a running cluster

On 03/04/2026 18:33, Daniel Gustafsson wrote:
> The attached rebase with a PG_CONTROL_VERSION bump is what I have staged for
> later tonight, submitting here to have the (hopefully) final patch archived as
> well as another CFBot run.

A few more small comments, I'm sorry about drip-feeding these:

> +/*
> + * launcher_exit
> + *
> + * Internal routine for cleaning up state when the launcher process exits. 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).
> + */
> +static void
> +launcher_exit(int code, Datum arg)
> +{
> + abort_requested = false;
> +
> + if (launcher_running)
> + {
> + LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
> + launcher_running = false;
> + DataChecksumState->launcher_running = false;
> +
> + if (DataChecksumState->worker_running != InvalidPid)
> + {
> + ereport(LOG,
> + errmsg("data checksums launcher exiting while worker is still running, signalling worker"));
> + kill(DataChecksumState->worker_running, SIGTERM);
> + }
> + LWLockRelease(DataChecksumsWorkerLock);
> + }
> +
> + /*
> + * If the launcher is exiting before data checksums are enabled then set
> + * the state to off since processing cannot be resumed.
> + */
> + if (DataChecksumsInProgress())
> + SetDataChecksumsOff();
> +}

Is there still a race condition if the launcher is killed, it gets here,
sends SIGTERM to the worker process, but before the worker process has
exited, the user calls pg_enable_data_checksums() again and a new
launcher is started? What happens?

> + /*
> + * Is a worker process currently running? This is set by the worker
> + * launcher when it starts waiting for a worker process to finish.
> + */
> + int worker_running;

'worker_running' sounds like a boolean, but it's actually a PID.
Especially when 'launcher_running' really is a boolean. Maybe rename to
'worker_pid' or 'worker_running_pid' or 'running_worker_pid' or something.

> +bool
> +DataChecksumsInProgress(void)
> +{
> + return LocalDataChecksumState == PG_DATA_CHECKSUM_INPROGRESS_ON;
> +}

Perhaps this should be DataChecksumsInProgressOn()? I saw the caller in
launcher_exit() first, and had to look up the implementation to check if
it returns true for 'inprogress-off' state.

> diff --git a/src/include/storage/checksum.h b/src/include/storage/checksum.h
> index ff417d5ae3e..fe5d30b4349 100644
> --- a/src/include/storage/checksum.h
> +++ b/src/include/storage/checksum.h
> @@ -15,6 +15,20 @@
>
> #include "storage/block.h"
>
> +/*
> + * Checksum state 0 is used for when data checksums are disabled (OFF).
> + * PG_DATA_CHECKSUM_INPROGRESS_{ON|OFF} defines that data checksums are either
> + * currently being enabled or disabled, and PG_DATA_CHECKSUM_VERSION defines
> + * that data checksums are enabled.
> + */
> +typedef enum ChecksumStateType
> +{
> + PG_DATA_CHECKSUM_OFF = 0,
> + PG_DATA_CHECKSUM_VERSION,
> + PG_DATA_CHECKSUM_INPROGRESS_OFF,
> + PG_DATA_CHECKSUM_INPROGRESS_ON,
> +} ChecksumStateType;
> +
> /*
> * Compute the checksum for a Postgres page. The page must be aligned on a
> * 4-byte boundary.

It'd be good to mention that this value is stored in the control file,
so changing it needs a catversion bump. Also it's important that
PG_DATA_CHECKSUM_VERSION = 1, for backwards-compatibility in pg_upgrade.
I'd suggest assigning explicit values 1, 2, 3, 4 for each of the enum
constants, to emphasize that they values are fixed.

There's an #include "storage/bufpage.h" in
src/bin/pg_upgrade/controldata.c and
src/backend/access/rmgrdesc/xlogdesc.c. I think they should both include
"storage/checksum.h" directly instead. And "bufpage.h" probably doesn't
need to include "storage/checksum.h".

- Heikki