public inbox for [email protected]
help / color / mirror / Atom feedFrom: Heikki Linnakangas <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Bernd Helmle <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Michael Banck <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Changing the state of data checksums in a running cluster
Date: Fri, 3 Apr 2026 19:00:52 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
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
view thread (60+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Changing the state of data checksums in a running cluster
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox