Message-ID: <c92b5d8b-bc03-47bc-b209-2e4a719eee32@iki.fi>
Date: Sat, 28 Mar 2026 01:30:56 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Changing the state of data checksums in a running cluster
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>
References: <7357A519-3391-4F07-BC69-5C9C2B120792@yesql.se>
 <3e67160c-3676-4419-b635-1fdb80dc128e@vondra.me>
 <67726BDA-2F21-4D93-B01F-FA68212ED6F0@yesql.se>
 <9CA2495B-2D6C-4308-97EF-A12CCCAA753E@yesql.se>
 <c33d8d86-47ad-4cee-9860-26b324d7306b@vondra.me>
 <477897AE-1314-4724-9694-0BABC4F4ABDA@yesql.se>
 <5mv5vtbqj2xm2wmevsi22smyn32jtznva3ya3daih3ixh3onex@s5fyab63xt3c>
 <2535E72D-5979-4F58-A2D3-2AA4AA335C20@yesql.se>
 <DEE0237A-D38C-477F-997C-3B0EB2D4FC57@yesql.se>
 <775fddbc-4dec-413d-89bb-1a8186858ee6@vondra.me>
 <683618b7-0946-400e-8066-97067f32d448@vondra.me>
 <C458D1B0-EF58-4DFC-9833-BC23D6B9C7F2@yesql.se>
 <daa95f1b-4dbd-4e03-a436-3ddd48b0fe56@vondra.me>
 <032619A2-D466-4A12-A524-98359D96AEA6@yesql.se>
 <300822bd-daeb-49e5-8af7-d7ce3af65adc@iki.fi>
 <BE93475D-504E-468E-867F-883749092DB9@yesql.se>
 <46d52594-fa93-4fbe-ad97-dc3127a04dd8@iki.fi>
 <EC4ED8A7-A9C5-4D92-B528-1303E97EEB48@yesql.se>
 <8B86AFB8-ACBC-44FC-A1AB-A58DB541A170@yesql.se>
 <452D5390-4F89-4DED-9835-8E9EA2A01E19@yesql.se>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <452D5390-4F89-4DED-9835-8E9EA2A01E19@yesql.se>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/c92b5d8b-bc03-47bc-b209-2e4a719eee32%40iki.fi>
Precedence: bulk

On 28/03/2026 00:03, Daniel Gustafsson wrote:
> The attached rebase contains lots more polish, mostly renaming variable names
> for clarity, tidying up comments and documentation and some smaller bits of
> cleanup like moving more code out of xlog.c.
> 
> This version runs all the tests in a normal test-run, with a few of them pared
> down with larger runs gated by PG_TEST_EXTRA.  I thinkt the tests are still too
> expensive in the event of getting committed, but it's helpful to have them
> during dev and test.  Executing pgbench sometimes fails in CI but I've been
> unable to reproduce that so not entirely sure what is going on there.
> 
> Heikki, Andres and Tomas; as you have been reviewing this patchset, what do you
> feel is left for considering this for commit?  (Apart from figuring out the CI
> test thing mentioned above which I think is a buildsystem issue.) I think 0001
> could be considered independently of 0002 and is cleanup in it's own right.

+1 for committing 0001 right away.


Some leftover stuff remains, related to the change that it no longer 
rebuilds the list of databases:

> + * The DataChecksumsWorker will compile a list of all databases at the start,
> + * and once all are processed will regenerate the list and start over
> + * processing any new entries. Once there are no new entries on the list,
> + * processing will end.  The regenerated list is required since databases can
> + * be created concurrently with data checksum processing, using a template
> + * database which has yet to be processed. All databases MUST BE successfully
> + * processed in order for data checksums to be enabled, the only exception are
> + * databases which are dropped before having been processed.

and here:

> +/*
> + * Test to remove an entry from the Databaselist to force re-processing since
> + * not all databases could be processed in the first iteration of the loop.
> + */
> +void
> +dc_dblist(const char *name, const void *private_data, void *arg)

The above talks about re-processing, but that doesn't happen anymore. I 
suspect the test that uses this is now obsolete or broken.

> +	while (true)
> +	{
> +		int			processed_databases = 0;
> +
> +		foreach_ptr(DataChecksumsWorkerDatabase, db, DatabaseList)
> +		{
> +			DataChecksumsWorkerResult result;
> +			DataChecksumsWorkerResultEntry *entry;
> +			bool		found;
> +
> +			/*
> +			 * Check if this database has been processed already, and if so
> +			 * whether it should be retried or skipped.
> +			 */
> +			entry = (DataChecksumsWorkerResultEntry *) hash_search(ProcessedDatabases, &db->dboid,
> +																   HASH_FIND, NULL);
> +

I guess this works, but wouldn't it be simpler to remove entries from 
DatabaseList when they're successfully processed?

> --- a/src/backend/commands/dbcommands.c
> +++ b/src/backend/commands/dbcommands.c
> @@ -1044,7 +1044,14 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
>  		if (pg_strcasecmp(strategy, "wal_log") == 0)
>  			dbstrategy = CREATEDB_WAL_LOG;
>  		else if (pg_strcasecmp(strategy, "file_copy") == 0)
> +		{
> +			if (DataChecksumsInProgress())
> +				ereport(ERROR,
> +						errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +						errmsg("create database strategy \"%s\" not allowed when data checksums are being enabled",
> +							   strategy));
>  			dbstrategy = CREATEDB_FILE_COPY;
> +		}
>  		else
>  			ereport(ERROR,
>  					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),

Is this enough? Is it possible that you start CREATE DATABASE in 
file_copy mode, and while it's already running but hasn't copied 
everything yet, you turn checksums on?

- Heikki