From 3b6df2f9c4acc0ee1b43e98a6379475c0e902acd Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Tue, 18 Aug 2026 08:04:12 +0000 Subject: [PATCH 2/2] Retry databases whose data checksums worker was terminated When enabling data checksums, a worker which exited without reporting a result is treated as a processing failure, which aborts the whole operation and reverts the checksum state to off. Commit 51f55b13a4d made the launcher treat a killed worker whose database is gone as a concurrent drop, but when a DROP DATABASE ... WITH (FORCE) kills the worker and then fails to drop the database, for example by exceeding the five seconds it waits for terminated backends to exit, the database still exists and the enable is aborted. The same happens when an administrator terminates a worker by hand. A terminated worker did not fail to process its database, so throwing away the whole operation is both surprising and unnecessary. To fix, record in shared memory when the worker is terminated by SIGTERM, and make the launcher retry the database rather than abort when the database still exists. Each retry requires another explicit termination, so this cannot loop on its own. A worker failing with an error still aborts the operation, and canceling the launcher still aborts it as well. Discussion: https://postgr.es/m/361531e2-52b5-499c-a126-815f277bbef2@gmail.com --- src/backend/postmaster/datachecksum_state.c | 85 ++++++++++++++++--- .../modules/test_checksums/t/001_basic.pl | 55 ++++++++++++ 2 files changed, 127 insertions(+), 13 deletions(-) diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c index f69258bc33d..6088fbcf721 100644 --- a/src/backend/postmaster/datachecksum_state.c +++ b/src/backend/postmaster/datachecksum_state.c @@ -290,6 +290,7 @@ typedef enum DATACHECKSUMSWORKER_ABORTED, DATACHECKSUMSWORKER_FAILED, DATACHECKSUMSWORKER_DROPDB, + DATACHECKSUMSWORKER_TERMINATED, } DataChecksumsWorkerResult; /* @@ -349,6 +350,14 @@ typedef struct DataChecksumsStateStruct /* result, set by worker before exiting */ DataChecksumsWorkerResult worker_result; + /* + * Set by the worker's SIGTERM handler, to let the launcher tell a + * terminated worker from one which hit an error. Written from a signal + * handler so not protected by the lock; the launcher resets it before + * starting a worker and reads it only after the worker has exited. + */ + volatile sig_atomic_t worker_terminated; + /* * Tells the worker process whether it should also process the shared * catalogs @@ -394,6 +403,7 @@ static BgwHandleStatus WaitForDataChecksumsWorkerState(BackgroundWorkerHandle *h static DataChecksumsWorkerResult ProcessDatabase(DataChecksumsWorkerDatabase *db); static void launcher_exit(int code, Datum arg); static void launcher_cancel_handler(SIGNAL_ARGS); +static void worker_terminate_handler(SIGNAL_ARGS); static void WaitForAllTransactionsToFinish(void); static bool ProcessAllDatabases(void); static void DataChecksumsShmemRequest(void *arg); @@ -961,6 +971,7 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db) */ DataChecksumState->worker_result = DATACHECKSUMSWORKER_FAILED; DataChecksumState->worker_pid = InvalidPid; + DataChecksumState->worker_terminated = false; invocation = ++DataChecksumState->worker_invocation_counter; DataChecksumState->worker_invocation = invocation; @@ -1029,12 +1040,17 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db) /* * Heuristic to see if the database was dropped, and if it was we can - * treat it as not an error, else treat as fatal and error out. + * treat it as not an error, else treat as fatal and error out. A + * worker terminated this early, e.g. by a DROP DATABASE ... WITH + * (FORCE) arriving while it was still connecting, is not an error + * either but the database has to be retried. */ - if (DatabaseExists(db->dboid)) - return DATACHECKSUMSWORKER_FAILED; - else + if (!DatabaseExists(db->dboid)) return DATACHECKSUMSWORKER_DROPDB; + else if (DataChecksumState->worker_terminated) + return DATACHECKSUMSWORKER_TERMINATED; + else + return DATACHECKSUMSWORKER_FAILED; } /* @@ -1085,9 +1101,17 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db) * likely FATALed in InitPostgres. If the database was dropped after we * built the database list then that is the expected outcome and not an * error, so apply the same heuristic as when the worker failed to start. + * + * If the database still exists but the worker was terminated, e.g. by a + * DROP DATABASE ... WITH (FORCE) which then failed to drop the database, + * the worker did not hit an error either, but the database still has to + * be processed. Report that separately so the launcher can retry it. */ if (result == DATACHECKSUMSWORKER_FAILED && !DatabaseExists(db->dboid)) result = DATACHECKSUMSWORKER_DROPDB; + else if (result == DATACHECKSUMSWORKER_FAILED && + DataChecksumState->worker_terminated) + result = DATACHECKSUMSWORKER_TERMINATED; CHECK_FOR_LAUNCHER_ABORT_REQUEST(); if (abort_requested) @@ -1166,6 +1190,23 @@ launcher_cancel_handler(SIGNAL_ARGS) errno = save_errno; } +/* + * worker_terminate_handler + * + * SIGTERM handler for the worker process. Record that the worker is being + * terminated before performing the normal die() processing, so that the + * launcher can tell a terminated worker from one which hit an error: a + * terminated worker is not a processing failure and its database is retried, + * while an error aborts the whole operation. + */ +static void +worker_terminate_handler(SIGNAL_ARGS) +{ + DataChecksumState->worker_terminated = true; + + die(postgres_signal_arg, pg_siginfo); +} + /* * WaitForAllTransactionsToFinish * Blocks awaiting all current transactions to finish @@ -1431,18 +1472,36 @@ ProcessAllDatabases(void) { DataChecksumsWorkerResult result; - result = ProcessDatabase(db); + for (;;) + { + result = ProcessDatabase(db); #ifdef USE_INJECTION_POINTS - /* Allow a test process to alter the result of the operation */ - if (IS_INJECTION_POINT_ATTACHED("datachecksumsworker-fail-db-result")) - { - result = DATACHECKSUMSWORKER_FAILED; - INJECTION_POINT_CACHED("datachecksumsworker-fail-db-result", - db->dbname); - } + /* Allow a test process to alter the result of the operation */ + if (IS_INJECTION_POINT_ATTACHED("datachecksumsworker-fail-db-result")) + { + result = DATACHECKSUMSWORKER_FAILED; + INJECTION_POINT_CACHED("datachecksumsworker-fail-db-result", + db->dbname); + } #endif + if (result != DATACHECKSUMSWORKER_TERMINATED) + break; + + /* + * The worker was terminated without hitting an error, and the + * database still exists. Start a new worker for it, since the + * database has to be processed for the operation to complete. + * Each retry requires another explicit termination, so this + * cannot loop on its own, and aborting the operation remains + * possible by canceling the launcher. + */ + ereport(LOG, + errmsg("data checksum processing was interrupted in database \"%s\", retrying", + db->dbname)); + } + pgstat_progress_update_param(PROGRESS_DATACHECKSUMS_DBS_DONE, ++cumulative_total); @@ -1747,7 +1806,7 @@ DataChecksumsWorkerMain(Datum arg) operation = ENABLE_DATACHECKSUMS; - pqsignal(SIGTERM, die); + pqsignal(SIGTERM, worker_terminate_handler); pqsignal(SIGUSR1, procsignal_sigusr1_handler); BackgroundWorkerUnblockSignals(); diff --git a/src/test/modules/test_checksums/t/001_basic.pl b/src/test/modules/test_checksums/t/001_basic.pl index 53fe8d70dc9..f1618daa04c 100644 --- a/src/test/modules/test_checksums/t/001_basic.pl +++ b/src/test/modules/test_checksums/t/001_basic.pl @@ -183,6 +183,61 @@ $node->poll_query_until('postgres', $result = $node->safe_psql('postgres', "SELECT count(*) FROM t WHERE a > 1"); is($result, '10000', 'ensure checksummed pages can be read back'); +# A worker terminated while its database still exists, for example by a +# DROP DATABASE ... WITH (FORCE) which then failed to drop the database, did +# not fail to process it. The launcher must retry the database rather than +# abort the whole operation. +disable_data_checksums($node, wait => 1); + +$node->safe_psql('postgres', "CREATE DATABASE killme;"); +$node->safe_psql('killme', + "CREATE TABLE killme_t AS SELECT generate_series(1,10000) AS a;"); + +# Hold the worker inside "killme" by keeping a temporary table around. +$bg = $node->background_psql('killme'); +$bg->query_safe('CREATE TEMP TABLE holdme (a int);'); + +enable_data_checksums($node); + +$node->poll_query_until( + 'postgres', qq[ + SELECT count(*) > 0 FROM pg_stat_activity + WHERE backend_type = 'datachecksums worker' AND datname = 'killme' + AND query LIKE 'Waiting for % temp tables to be removed'] +) or die "timed out waiting for worker to wait for temporary tables"; + +my $worker_pid = $node->safe_psql( + 'postgres', qq[ + SELECT pid FROM pg_stat_activity + WHERE backend_type = 'datachecksums worker' AND datname = 'killme']); +$node->safe_psql('postgres', "SELECT pg_terminate_backend($worker_pid);"); + +# The launcher starts a replacement worker, which waits for the temp table +# just like the terminated one did. +$node->poll_query_until( + 'postgres', qq[ + SELECT count(*) > 0 FROM pg_stat_activity + WHERE backend_type = 'datachecksums worker' AND datname = 'killme' + AND pid <> $worker_pid + AND query LIKE 'Waiting for % temp tables to be removed'] +) or die "timed out waiting for replacement worker"; + +$log = slurp_file($node->logfile); +like( + $log, + qr/data checksum processing was interrupted in database "killme", retrying/, + 'retry of the terminated worker is logged'); + +# Release the replacement worker and let the operation complete. +$bg->query_safe('DROP TABLE holdme;'); +$bg->quit; + +wait_for_checksum_state($node, 'on'); +$node->poll_query_until('postgres', + "SELECT count(*) = 0 " + . "FROM pg_catalog.pg_stat_activity " + . "WHERE backend_type = 'datachecksums launcher';"); + $node->stop; # The resulting cluster must also pass offline verification, proving no -- 2.54.0