From d48ddd8a945f2ed3be5251e9b923de93f370c188 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 13 Aug 2026 16:37:01 +0200 Subject: [PATCH v3 1/5] Make data checksums launcher cancel its worker at SIGINT Make sure the launcher cancels the currently running worker by calling TerminateBackgroundWorker when it receives SIGINT. In order to handle cases where SIGINT arrives while tje launcher is waiting for a worker to start or exit, implement a version of WaitForBackgroundWorkerStartup and WaitForBackgroundWorkerShutdown which checks the abort_requested signalling. A new test which kills processing with SIGINT is added, and while in there remove a stray call to disable checksums before the test ends which isn't required and schema qualify an existing query against pg_stat_activity. Backpatch to v19 where online checksums were introduced. Author: Fujii Masao Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/CAHGQGwEQ1-+iPQnUpTXYiHmzSz9ufFVkOK4kL_uyTdYt7jgg0Q@mail.gmail.com Backpatch-through: 19 --- src/backend/postmaster/datachecksum_state.c | 112 ++++++++++++++++-- .../modules/test_checksums/t/002_restarts.pl | 46 ++++++- 2 files changed, 144 insertions(+), 14 deletions(-) diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c index be308a282d5..7dfe7c812bb 100644 --- a/src/backend/postmaster/datachecksum_state.c +++ b/src/backend/postmaster/datachecksum_state.c @@ -393,6 +393,10 @@ static DataChecksumsWorkerResult ProcessDatabase(DataChecksumsWorkerDatabase *db static bool ProcessAllDatabases(void); static bool ProcessSingleRelationFork(Relation reln, ForkNumber forkNum, BufferAccessStrategy strategy); static void ResetDataChecksumsProgressCounters(void); +static BgwHandleStatus WaitForDataChecksumsWorkerState(BackgroundWorkerHandle *handle, + bool wait_for_startup, + pid_t *pidp, + uint32 wait_event); static void launcher_cancel_handler(SIGNAL_ARGS); static void WaitForAllTransactionsToFinish(void); @@ -861,6 +865,73 @@ ProcessSingleRelationByOid(Oid relationId, BufferAccessStrategy strategy) return !aborted; } +/* + * WaitForDataChecksumsWorkerState + * Wait for a data checksums worker to start or stop. + * + * This is like WaitForBackgroundWorkerStartup() and + * WaitForBackgroundWorkerShutdown(), except that it also reacts to SIGINT + * received by the launcher. The launcher owns the overall checksum + * operation, so canceling it should stop the worker it has registered or is + * currently running. + * + * If wait_for_startup is true, wait until the worker is no longer in + * BGWH_NOT_YET_STARTED state, like WaitForBackgroundWorkerStartup(). If it + * is false, wait until the worker reaches BGWH_STOPPED state, like + * WaitForBackgroundWorkerShutdown(). + * + * pidp is set to the worker's PID when startup succeeds, if it is not NULL. + */ +static BgwHandleStatus +WaitForDataChecksumsWorkerState(BackgroundWorkerHandle *handle, + bool wait_for_startup, + pid_t *pidp, + uint32 wait_event) +{ + BgwHandleStatus status; + bool termination_requested = false; + + for (;;) + { + int rc; + pid_t pid; + + CHECK_FOR_INTERRUPTS(); + + status = GetBackgroundWorkerPid(handle, &pid); + if (status == BGWH_STARTED && pidp) + *pidp = pid; + + if (abort_requested && !termination_requested) + { + TerminateBackgroundWorker(handle); + termination_requested = true; + } + + /* + * Startup waits for the worker to leave BGWH_NOT_YET_STARTED, while + * shutdown waits for it to reach BGWH_STOPPED. + */ + if (status == BGWH_STOPPED || + (wait_for_startup && status == BGWH_STARTED)) + break; + + rc = WaitLatch(MyLatch, + WL_LATCH_SET | WL_POSTMASTER_DEATH, 0, + wait_event); + + if (rc & WL_POSTMASTER_DEATH) + { + status = BGWH_POSTMASTER_DIED; + break; + } + + ResetLatch(MyLatch); + } + + return status; +} + /* * ProcessDatabase * Enable data checksums in a single database. @@ -921,9 +992,20 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db) return DATACHECKSUMSWORKER_FAILED; } - status = WaitForBackgroundWorkerStartup(bgw_handle, &pid); + /* + * While this expects to wait for BGWORKER_STARTUP it may return _STOPPED + * if the worker was terminated in the meantime so we must check status. + */ + status = WaitForDataChecksumsWorkerState(bgw_handle, true, &pid, + WAIT_EVENT_BGWORKER_STARTUP); if (status == BGWH_STOPPED) { + if (abort_requested) + { + result = DATACHECKSUMSWORKER_ABORTED; + goto done; + } + /* * If the worker managed to start, and stop, before we got to waiting * for it we can see a STOPPED status here without it being a failure. @@ -981,7 +1063,8 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db) "Waiting for worker in database %s (pid %ld)", db->dbname, (long) pid); pgstat_report_activity(STATE_RUNNING, activity); - status = WaitForBackgroundWorkerShutdown(bgw_handle); + status = WaitForDataChecksumsWorkerState(bgw_handle, false, NULL, + WAIT_EVENT_BGWORKER_SHUTDOWN); if (status == BGWH_POSTMASTER_DIED) ereport(FATAL, errcode(ERRCODE_ADMIN_SHUTDOWN), @@ -1004,6 +1087,11 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db) if (result == DATACHECKSUMSWORKER_FAILED && !DatabaseExists(db->dboid)) result = DATACHECKSUMSWORKER_DROPDB; + CHECK_FOR_LAUNCHER_ABORT_REQUEST(); + if (abort_requested) + result = DATACHECKSUMSWORKER_ABORTED; + +done: if (result == DATACHECKSUMSWORKER_ABORTED) ereport(LOG, errmsg("data checksums processing was aborted in database \"%s\"", @@ -1056,9 +1144,9 @@ launcher_exit(int code, Datum arg) /* * launcher_cancel_handler * - * Internal routine for reacting to SIGINT and flagging the worker to abort. - * The worker won't be interrupted immediately but will check for abort flag - * between each block in a relation. + * Internal routine for reacting to SIGINT and flagging the launcher to abort. + * If a worker is registered or running, the launcher will request worker + * termination from its normal control flow. */ static void launcher_cancel_handler(SIGNAL_ARGS) @@ -1068,10 +1156,8 @@ launcher_cancel_handler(SIGNAL_ARGS) abort_requested = true; /* - * There is no sleeping in the main loop, the flag will be checked - * periodically in ProcessSingleRelationFork. The worker does however - * sleep when waiting for concurrent transactions to end so we still need - * to set the latch. + * Wake the launcher if it is waiting for transactions to finish or for a + * worker to start up or shut down. */ SetLatch(MyLatch); @@ -1226,8 +1312,9 @@ again: if (!ProcessAllDatabases()) { /* - * If the target state changed during processing then it's not a - * failure, so restart processing instead. + * If processing was canceled, or the target state changed during + * processing, then it's not a failure. In the latter case, the + * launcher will restart processing with the new target state. */ CHECK_FOR_LAUNCHER_ABORT_REQUEST(); if (abort_requested) @@ -1314,6 +1401,8 @@ ProcessAllDatabases(void) /* Get a list of all databases to process */ WaitForAllTransactionsToFinish(); + if (abort_requested) + return false; DatabaseList = BuildDatabaseList(); /* @@ -1370,6 +1459,7 @@ ProcessAllDatabases(void) else if (result == DATACHECKSUMSWORKER_ABORTED || abort_requested) { /* Abort flag set, so exit the whole process */ + FreeDatabaseList(DatabaseList); return false; } else if (result == DATACHECKSUMSWORKER_DROPDB) diff --git a/src/test/modules/test_checksums/t/002_restarts.pl b/src/test/modules/test_checksums/t/002_restarts.pl index 1aa2c0c65e5..27b080c7f0b 100644 --- a/src/test/modules/test_checksums/t/002_restarts.pl +++ b/src/test/modules/test_checksums/t/002_restarts.pl @@ -93,6 +93,48 @@ SKIP: test_checksum_state($node, 'off'); } +# Test interrupting the processing with SIGINT to make sure the launcher and +# worker are cancelled. Create a barrier for checksum enablement to block on +# using the same technique as earlier with a temporary table. +my $block_session = $node->background_psql('postgres'); +$block_session->query_safe('CREATE TEMPORARY TABLE tt (a integer);'); + +# In another session, make sure we can see the blocking temp table but +# start processing anyways and check that we are blocked with a proper +# wait event. +$result = $node->safe_psql('postgres', + "SELECT relpersistence FROM pg_catalog.pg_class WHERE relname = 'tt';"); +is($result, 't', 'ensure we can see the temporary table'); + +# Ensure that we reach inprogress-on and the worker is launched and waiting for +# the temporary table to disappear before we try to interrupt the processing. +enable_data_checksums($node, wait => 'inprogress-on'); +$result = $node->poll_query_until( + 'postgres', + "SELECT wait_event FROM pg_catalog.pg_stat_activity " + . "WHERE backend_type = 'datachecksums worker';", + 'ChecksumEnableTemptableWait'); +is($result, '1', 'ensure the correct wait condition is set'); + +# Terminate launcher and worker with SIGINT +my $pid = $node->safe_psql('postgres', + "SELECT pid FROM pg_catalog.pg_stat_activity WHERE backend_type = 'datachecksums launcher';" +); +is(PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'INT', $pid), + 0, "datachecksums launcher process signalled with INT"); + +# Wait for all processes to exit and make sure that the data_checksum state +# was reverted back to off since we didn't finish +$result = $node->poll_query_until( + 'postgres', + "SELECT count(*) FROM pg_catalog.pg_stat_activity WHERE backend_type LIKE 'datachecksums%';", + '0'); +is($result, 1, 'await datachecksums worker/launcher termination'); +wait_for_checksum_state($node, "off"); +$block_session->quit; + +# Finish test suite by enabling checksums and make sure all data can be read +# back and no processes are left over enable_data_checksums($node, wait => 'on'); $result = $node->safe_psql('postgres', "SELECT count(*) FROM t WHERE a > 1"); @@ -100,11 +142,9 @@ is($result, '9999', 'ensure checksummed pages can be read back'); $result = $node->poll_query_until( 'postgres', - "SELECT count(*) FROM pg_stat_activity WHERE backend_type LIKE 'datachecksums%';", + "SELECT count(*) FROM pg_catalog.pg_stat_activity WHERE backend_type LIKE 'datachecksums%';", '0'); is($result, 1, 'await datachecksums worker/launcher termination'); -disable_data_checksums($node, wait => 1); - $node->stop; done_testing(); -- 2.39.3 (Apple Git-146)