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

It'd be good to print a LOG message when the checksums state changes, to 
have a trail in the log of when checksums were enabled/disabled. 
Something like:

LOG:  enabling checksums was requested, starting checksum calculation
...
LOG:  checksums calculation finished, checksums are now enabled


On 02/04/2026 02:01, Daniel Gustafsson wrote:
> +        if (result == DATACHECKSUMSWORKER_FAILED)
> +        {
> +            /*
> +             * Disable checksums on cluster, because we failed one of the
> +             * databases and this is an all or nothing process.
> +             */
> +            SetDataChecksumsOff();
> +            ereport(ERROR,
> +                    errcode(ERRCODE_INSUFFICIENT_RESOURCES),
> +                    errmsg("data checksums failed to get enabled in all databases, aborting"),
> +                    errhint("The server log might have more information on the cause of the error."));
> +        }

This got me thinking, what happens if the the data checksums launcher 
encounters some other error, for example if you SIGTERM it? The system 
is left in 'inprogress-on' state, but because the launcher is gone it 
will never finish and 'pg_stat_progress_data_checksums' will be empty.

Perhaps launcher_exit() should call SetDataChecksumsOff()?

>         /*
>          * TODO: how to really handle the worker still running when the
>          * launcher exits?
>          */
>         if (DataChecksumsWorkerShmem->worker_running)
>             ereport(LOG,
>                     errmsg("data checksums launcher exiting while worker is still running"));

That TODO should be addressed somehow.

> +        /*
> +         * As of now we only update the block counter for main forks in order
> +         * to not cause too frequent calls. TODO: investigate whether we
> +         * should do it more frequent?
> +         */
> +        if (forkNum == MAIN_FORKNUM)
> +            pgstat_progress_update_param(PROGRESS_DATACHECKSUMS_BLOCKS_DONE,
> +                                         (blknum + 1));

We're updating it for every block in the main fork, but not at all for 
other forks? What a bizarre way of avoiding too frequent calls :-). I 
think you could just call this on every page, 
pgstat_progress_update_param() is supposed to be very fast. For 
comparison, e.g. index build calls it for every tuple.

> +/*
> + * Configuration of conditions which must match when absorbing a procsignal
> + * barrier during data checksum enable/disable operations.  A single function
> + * is used for absorbing all barriers, and the set of conditions to use is
> + * looked up in the checksum_barriers struct.  The struct member for the target
> + * state defines which state the backend must currently be in, and which it
> + * must not be in.
> + *
> + * The reason for this explicit checking is to ensure that processing cannot
> + * be started such that it breaks the assumptions of the state machine.
> + *
> + * MAX_BARRIER_CONDITIONS must match largest number of sets in barrier_eq and
> + * barrier_ne in the below checksum_barriers definition.
> + */
> +#define MAX_BARRIER_CONDITIONS 2
> +typedef struct ChecksumBarrierCondition
> +{
> +    /* The target state of the barrier */
> +    int            target;
> +    /* A set of states in which at least one MUST match the current state */
> +    int            barrier_eq[MAX_BARRIER_CONDITIONS];
> +    /* The number of elements in the barrier_eq set */
> +    int            barrier_eq_sz;
> +    /* A set of states which all MUST NOT match the current state */
> +    int            barrier_ne[MAX_BARRIER_CONDITIONS];
> +    /* The number of elements in the barrier_ne set */
> +    int            barrier_ne_sz;
> +} ChecksumBarrierCondition;
> +
> +static const ChecksumBarrierCondition checksum_barriers[4] =
> +{
> +    /*
> +     * When disabling checksums, either inprogress state is Ok but checksums
> +     * must not be in the enabled state.
> +     */
> +    {
> +        .target = PG_DATA_CHECKSUM_OFF,
> +        .barrier_eq = {PG_DATA_CHECKSUM_INPROGRESS_ON, PG_DATA_CHECKSUM_INPROGRESS_OFF},
> +        .barrier_eq_sz = 2,
> +        .barrier_ne = {PG_DATA_CHECKSUM_VERSION},
> +        .barrier_ne_sz = 1
> +    },
> +    /* When enabling the current state must be inprogress-on */
> +    {
> +        .target = PG_DATA_CHECKSUM_VERSION,
> +        .barrier_eq = {PG_DATA_CHECKSUM_INPROGRESS_ON},
> +        .barrier_eq_sz = 1,
> +        {0}, 0
> +    },
> +
> +    /*
> +     * When moving to inprogress-on the current state cannot enabled, but when
> +     * moving to inprogress-off the current state must be enabled.
> +     */
> +    {
> +        .target = PG_DATA_CHECKSUM_INPROGRESS_ON,
> +        {0}, 0,
> +        .barrier_ne = {PG_DATA_CHECKSUM_VERSION},
> +        .barrier_ne_sz = 1
> +    },
> +    {
> +        .target = PG_DATA_CHECKSUM_INPROGRESS_OFF,
> +        .barrier_eq = {PG_DATA_CHECKSUM_VERSION},
> +        .barrier_eq_sz = 1,
> +        {0}, 0
> +    },
> +};

I find this to still be a pretty complicated and unclear way of 
representing the allowed transitions. There are only 16 possible 
transitions, and only 6 of them are allowed. How about listing the 
allowed ones directly:

/* Allowed transitions: from, to */
{
     /*
      * Disabling checksums: If checksums are currently enabled,
      * disabling must go through the 'inprogress-off' state.
      */
     {PG_DATA_CHECKSUM_VERSION, PG_DATA_CHECKSUM_INPROGRESS_OFF},
     {PG_DATA_CHECKSUM_INPROGRESS_OFF, PG_DATA_CHECKSUM_OFF},

     /*
      * If checksums are in the process of being enabled, but are
      * not yet being verified, we can abort by going back to 'off'
      * state.
      */
     {PG_DATA_CHECKSUM_INPROGRESS_ON, PG_DATA_CHECKSUM_OFF},

     /*
      * Enabling checksums must normally go through the 'inprogress-on'
      * state.
      */
     {PG_DATA_CHECKSUM_OFF, PG_DATA_CHECKSUM_INPROGRESS_ON},
     {PG_DATA_CHECKSUM_INPROGRESS_ON, PG_DATA_CHECKSUM_VERSION},

     /*
      * If checksums are being disabled but all backends are still
      * computing checksums, we can go straight back to 'on'
      */
     {PG_DATA_CHECKSUM_INPROGRESS_OFF, PG_DATA_CHECKSUM_VERSION},
}

> +/*
> + * Signaling between backends calling pg_enable/disable_data_checksums, the
> + * checksums launcher process, and the checksums worker process.
> + *
> + * This struct is protected by DataChecksumsWorkerLock
> + */
> +typedef struct DataChecksumsWorkerShmemStruct
> +{
> +    /*
> +     * These are set by pg_{enable|disable|verify}_data_checksums, to tell the
> +     * launcher what the target state is.
> +     */
> +    DataChecksumsWorkerOperation launch_operation;
> +    int            launch_cost_delay;
> +    int            launch_cost_limit;

The naming feels a little weird with this struct. It's called 
"DataChecksumsWorkerShmemStruct", but it's also accessed by the backends 
calling pg_enable/disable_data_checksums(). And 
"DataChecksumsWorkerOperation" is not accessed by workers at all. Or I 
guess the "operation" global variable is used in 
DataChecksumsWorkerMain(), but it's always set to ENABLE_DATACHECKSUMS 
in the worker. Do you need the "operation" global variable at all?

> +void
> +SetDataChecksumsOn(void)
> +{
> +    uint64        barrier;
> +
> +    Assert(ControlFile != NULL);
> +
> +    SpinLockAcquire(&XLogCtl->info_lck);
> +
> +    /*
> +     * The only allowed state transition to "on" is from "inprogress-on" since
> +     * that state ensures that all pages will have data checksums written.
> +     */
> +    if (XLogCtl->data_checksum_version != PG_DATA_CHECKSUM_INPROGRESS_ON)
> +    {
> +        SpinLockRelease(&XLogCtl->info_lck);
> +        elog(PANIC, "checksums not in \"inprogress-on\" mode");
> +    }
> +
> +    SpinLockRelease(&XLogCtl->info_lck);

The PANIC seems a little harsh, you haven't done anything destructive 
here. It's unexpected for this to be called in any other state, so this 
is a "can't happen" scenario, but I don't think we usually PANIC on those.

> +       <para>
> +        If <parameter>cost_delay</parameter> and <parameter>cost_limit</parameter> are
> +        specified, the process is throttled using the same principles as
> +        <link linkend="runtime-config-resource-vacuum-cost">Cost-based Vacuum Delay</link>.
> +       </para>

Ugh, yet another place where we expose the "cost delay/limit" throttling 
mechanism. I agree it's good to be consistent here and use the same 
method we use for vacuum, I just wish we had something more user-friendly..


Grammar / spelling:

> + * state will also be set of "off".

> +     * When moving to inprogress-on the current state cannot enabled, but when

> +     * If a worker process currently running?  This is set by the worker

> +     * These are set by pg_{enable|disable|verify}_data_checksums, to tell the

there is no "pg_verify_data_checksums" function.

"calcuated" in commit message

- Heikki