Horton
Menu

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

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Tomas Vondra <tomas@vondra.me>, Andres Freund <andres@anarazel.de>, Bernd Helmle <mailings@oopsware.de>, Michael Paquier <michael@paquier.xyz>, Michael Banck <mbanck@gmx.net>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-04-03T16:00:52Z
Lists: 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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Use correct datatype for PID

  2. Improve comments in online checksums code

  3. Fix checksum state transition during promotion

  4. Fix regex searching for page verification failures in tests

  5. Apply data-checksum worker throttling parameters

  6. Skip WAL for unlogged main fork during online checksum enable

  7. Fix data_checksum GUC show_hook

  8. Improve database detection logic in datachecksumsworker

  9. Improve handling of concurrent checksum requests

  10. Typo and spelling fixups for online checksums

  11. Fix invalid checksum state transition in checkpoints

  12. Handle data_checksum state changes during launcher_exit

  13. Test improvements for online checksums

  14. Prevent pg_enable/disable_data_checksums() on standby

  15. Test stabilization for online checksums

  16. Make data checksum tests more resilient for slow machines

  17. Formalize WAL record for XLOG_CHECKPOINT_REDO

  18. Revert "Get rid of WALBufMappingLock"

  19. Get rid of WALBufMappingLock

  20. Improve grammar of options for command arrays in TAP tests