Re: Changing the state of data checksums in a running cluster - Mailing list pgsql-hackers

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