v2-0006-Improve-handling-of-concurrent-checksum-requests.patch

From 6aaf7739d439c99ef7aa8dcd6c916792a2aa1eef Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Tue, 28 Apr 2026 23:49:30 +0200
Subject: [PATCH v2 6/8] Improve handling of concurrent checksum requests

When pg_{enable|disable}_data_checksums is called while checksums are
being enabled or disabled, the already running launcher is detected
and the new desired state is recorded.  Processing will then pick up
the new state and change its operation to fulfill the new request.

If the same state is requested but updated cost values, the new cost
values will take effect on the next relation processed.  The previous
coding had a complex logic of starting a new launcher for this, which
is now avoided with the shared mem structure instead used to signal
current processing.

This makes the logic more robust, and fixes a bug where the launcher
would erroneously revert back to the "off" state.

Author: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/xxx
---
 src/backend/access/transam/xlog.c           | 33 +++++++-
 src/backend/postmaster/datachecksum_state.c | 85 +++++++++++++++------
 src/include/access/xlog.h                   |  2 +
 3 files changed, 96 insertions(+), 24 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7eb61158f26..757f309016c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4682,10 +4682,41 @@ DataChecksumsNeedWrite(void)
 			LocalDataChecksumState == PG_DATA_CHECKSUM_INPROGRESS_OFF);
 }
 
+
+bool
+DataChecksumsOff(void)
+{
+	bool		ret;
+
+	SpinLockAcquire(&XLogCtl->info_lck);
+	ret = (XLogCtl->data_checksum_version == PG_DATA_CHECKSUM_OFF);
+	SpinLockRelease(&XLogCtl->info_lck);
+
+	return ret;
+}
+
+bool
+DataChecksumsOn(void)
+{
+	bool		ret;
+
+	SpinLockAcquire(&XLogCtl->info_lck);
+	ret = (XLogCtl->data_checksum_version == PG_DATA_CHECKSUM_VERSION);
+	SpinLockRelease(&XLogCtl->info_lck);
+
+	return ret;
+}
+
 bool
 DataChecksumsInProgressOn(void)
 {
-	return LocalDataChecksumState == PG_DATA_CHECKSUM_INPROGRESS_ON;
+	bool		ret;
+
+	SpinLockAcquire(&XLogCtl->info_lck);
+	ret = (XLogCtl->data_checksum_version == PG_DATA_CHECKSUM_INPROGRESS_ON);
+	SpinLockRelease(&XLogCtl->info_lck);
+
+	return ret;
 }
 
 /*
diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c
index 27cdbb3ed1e..be1b0a2d926 100644
--- a/src/backend/postmaster/datachecksum_state.c
+++ b/src/backend/postmaster/datachecksum_state.c
@@ -235,7 +235,7 @@ typedef struct ChecksumBarrierCondition
 	int			to;
 } ChecksumBarrierCondition;
 
-static const ChecksumBarrierCondition checksum_barriers[7] =
+static const ChecksumBarrierCondition checksum_barriers[9] =
 {
 	/*
 	 * Disabling checksums: If checksums are currently enabled, disabling must
@@ -267,6 +267,13 @@ static const ChecksumBarrierCondition checksum_barriers[7] =
 	 * set to off since we cannot reach on at that point.
 	 */
 	{PG_DATA_CHECKSUM_INPROGRESS_ON, PG_DATA_CHECKSUM_INPROGRESS_OFF},
+
+	/*
+	 * Transitions that can happen when a new request is made while another is
+	 * currently being processed.
+	 */
+	{PG_DATA_CHECKSUM_INPROGRESS_OFF, PG_DATA_CHECKSUM_INPROGRESS_ON},
+	{PG_DATA_CHECKSUM_OFF, PG_DATA_CHECKSUM_INPROGRESS_OFF},
 };
 
 /*
@@ -377,6 +384,15 @@ const ShmemCallbacks DataChecksumsShmemCallbacks = {
 	.init_fn = DataChecksumsShmemInit,
 };
 
+#define CHECK_FOR_ABORT_REQUEST() \
+	do {															\
+		LWLockAcquire(DataChecksumsWorkerLock, LW_SHARED);			\
+		if (DataChecksumState->launch_operation != operation)		\
+			abort_requested = true;									\
+		LWLockRelease(DataChecksumsWorkerLock);						\
+	} while (0)
+
+
 /*****************************************************************************
  * Functionality for manipulating the data checksum state in the cluster
  */
@@ -566,7 +582,6 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
 	BackgroundWorker bgw;
 	BackgroundWorkerHandle *bgw_handle;
 	bool		running;
-	DataChecksumsWorkerOperation launcher_running_op;
 
 #ifdef USE_ASSERT_CHECKING
 	/* The cost delay settings have no effect when disabling */
@@ -585,8 +600,6 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
 
 	/* Is the launcher already running? If so, what is it doing? */
 	running = DataChecksumState->launcher_running;
-	if (running)
-		launcher_running_op = DataChecksumState->operation;
 
 	LWLockRelease(DataChecksumsWorkerLock);
 
@@ -603,13 +616,17 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
 	 * the launcher has had a chance to start up, we still end up launching it
 	 * twice.  That's OK, the second invocation will see that a launcher is
 	 * already running and exit quickly.
-	 *
-	 * TODO: We could optimize here and skip launching the launcher, if we are
-	 * already in the desired state, i.e. if the checksums are already enabled
-	 * and you call pg_enable_data_checksums().
 	 */
 	if (!running)
 	{
+		if ((op == ENABLE_DATACHECKSUMS && DataChecksumsOn()) ||
+			(op == DISABLE_DATACHECKSUMS && DataChecksumsOff()))
+		{
+			ereport(LOG,
+					errmsg("data checksums already in desired state, exiting"));
+			return;
+		}
+
 		/*
 		 * Prepare the BackgroundWorker and launch it.
 		 */
@@ -631,9 +648,8 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
 	}
 	else
 	{
-		if (launcher_running_op == op)
-			ereport(ERROR,
-					errmsg("data checksum processing already running"));
+		ereport(LOG,
+				errmsg("data checksum processing already running"));
 	}
 }
 
@@ -1024,11 +1040,8 @@ WaitForAllTransactionsToFinish(void)
 					errhint("Data checksums processing must be restarted manually after cluster restart."));
 
 		CHECK_FOR_INTERRUPTS();
+		CHECK_FOR_ABORT_REQUEST();
 
-		LWLockAcquire(DataChecksumsWorkerLock, LW_SHARED);
-		if (DataChecksumState->launch_operation != operation)
-			abort_requested = true;
-		LWLockRelease(DataChecksumsWorkerLock);
 		if (abort_requested)
 			break;
 	}
@@ -1212,7 +1225,9 @@ ProcessAllDatabases(void)
 	int			cumulative_total = 0;
 
 	/* Set up so first run processes shared catalogs, not once in every db */
+	LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
 	DataChecksumState->process_shared_catalogs = true;
+	LWLockRelease(DataChecksumsWorkerLock);
 
 	/* Get a list of all databases to process */
 	WaitForAllTransactionsToFinish();
@@ -1288,7 +1303,9 @@ ProcessAllDatabases(void)
 		 * When one database has completed, it will have done shared catalogs
 		 * so we don't have to process them again.
 		 */
+		LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
 		DataChecksumState->process_shared_catalogs = false;
+		LWLockRelease(DataChecksumsWorkerLock);
 	}
 
 	FreeDatabaseList(DatabaseList);
@@ -1538,7 +1555,6 @@ DataChecksumsWorkerMain(Datum arg)
 	 * implementation detail and care should be taken to avoid it bleeding
 	 * through to the user to avoid confusion.
 	 */
-	Assert(DataChecksumState->operation == ENABLE_DATACHECKSUMS);
 	VacuumCostDelay = DataChecksumState->cost_delay;
 	VacuumCostLimit = DataChecksumState->cost_limit;
 	VacuumCostActive = (VacuumCostDelay > 0);
@@ -1574,8 +1590,6 @@ DataChecksumsWorkerMain(Datum arg)
 	rels_done = 0;
 	foreach_oid(reloid, RelationList)
 	{
-		CHECK_FOR_INTERRUPTS();
-
 		if (!ProcessSingleRelationByOid(reloid, strategy))
 		{
 			aborted = true;
@@ -1584,12 +1598,38 @@ DataChecksumsWorkerMain(Datum arg)
 
 		pgstat_progress_update_param(PROGRESS_DATACHECKSUMS_RELS_DONE,
 									 ++rels_done);
+		CHECK_FOR_INTERRUPTS();
+		CHECK_FOR_ABORT_REQUEST();
+
+		if (abort_requested)
+			break;
+
+		/* Check if the cost settings changed during runtime */
+		LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
+		if ((DataChecksumState->launch_cost_delay != DataChecksumState->cost_delay)
+			|| (DataChecksumState->launch_cost_limit != DataChecksumState->cost_limit))
+		{
+			VacuumCostDelay = DataChecksumState->launch_cost_delay;
+			VacuumCostLimit = DataChecksumState->launch_cost_limit;
+			VacuumCostActive = (VacuumCostDelay > 0);
+
+			FreeAccessStrategy(strategy);
+			strategy = GetAccessStrategy(BAS_VACUUM);
+			DataChecksumState->cost_delay = DataChecksumState->launch_cost_delay;
+			DataChecksumState->cost_limit = DataChecksumState->launch_cost_limit;
+		}
+		LWLockRelease(DataChecksumsWorkerLock);
+
 	}
+
 	list_free(RelationList);
+	FreeAccessStrategy(strategy);
 
-	if (aborted)
+	if (aborted || abort_requested)
 	{
+		LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
 		DataChecksumState->success = DATACHECKSUMSWORKER_ABORTED;
+		LWLockRelease(DataChecksumsWorkerLock);
 		ereport(DEBUG1,
 				errmsg("data checksum processing aborted in database OID %u",
 					   dboid));
@@ -1654,15 +1694,14 @@ DataChecksumsWorkerMain(Datum arg)
 						 3000,
 						 WAIT_EVENT_CHECKSUM_ENABLE_TEMPTABLE_WAIT);
 
-		LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
-		aborted = DataChecksumState->launch_operation != operation;
-		LWLockRelease(DataChecksumsWorkerLock);
-
 		CHECK_FOR_INTERRUPTS();
+		CHECK_FOR_ABORT_REQUEST();
 
 		if (aborted || abort_requested)
 		{
+			LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
 			DataChecksumState->success = DATACHECKSUMSWORKER_ABORTED;
+			LWLockRelease(DataChecksumsWorkerLock);
 			ereport(LOG,
 					errmsg("data checksum processing aborted in database OID %u",
 						   dboid));
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 437b4f32349..4dd98624204 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -249,6 +249,8 @@ extern uint64 GetSystemIdentifier(void);
 extern char *GetMockAuthenticationNonce(void);
 extern bool DataChecksumsNeedWrite(void);
 extern bool DataChecksumsNeedVerify(void);
+extern bool DataChecksumsOn(void);
+extern bool DataChecksumsOff(void);
 extern bool DataChecksumsInProgressOn(void);
 extern void SetDataChecksumsOnInProgress(void);
 extern void SetDataChecksumsOn(void);
-- 
2.39.3 (Apple Git-146)