From 7517ce4a6c33be0f75780ade699afc1e807cf997 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 23 Apr 2026 10:44:32 +0200 Subject: [PATCH 6/8] Improve handling of concurrent checksum requests When pg_{enable|disable}_data_checksums is called while checksums are being enabled of disabled, the already running launcher is detected and the new desired state is recorded. Processing will then pick up the new state and change it's 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 ss 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 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 9b1a10dbd1c..49772510855 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 5e14803b0b0..c382b954d91 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}, }; /* @@ -368,6 +375,15 @@ const ShmemCallbacks DataChecksumsShmemCallbacks = { .request_fn = DataChecksumsShmemRequest, }; +#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 */ @@ -557,7 +573,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 */ @@ -576,8 +591,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); @@ -594,13 +607,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. */ @@ -622,9 +639,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")); } } @@ -998,11 +1014,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; } @@ -1185,7 +1198,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(); @@ -1261,7 +1276,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); @@ -1503,7 +1520,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); @@ -1539,8 +1555,6 @@ DataChecksumsWorkerMain(Datum arg) rels_done = 0; foreach_oid(reloid, RelationList) { - CHECK_FOR_INTERRUPTS(); - if (!ProcessSingleRelationByOid(reloid, strategy)) { aborted = true; @@ -1549,12 +1563,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)); @@ -1619,15 +1659,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)