From fafefbc4b429e7c187f7ccbdce88f4227ce7eb24 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 9 Nov 2026 12:31:49 +0900 Subject: [PATCH v4] Fix restart of interruptible bgworkers after database commands TerminateBackgroundWorkersForDatabase() set the terminate flag of the workers' slots, so the postmaster unregistered them regardless of bgw_restart_time, even if the database command failed afterwards. Let's signal the workers directly instead, like CountOtherDBBackends() does for autovacuum workers, so that they are restarted based on bgw_restart_time. This gives the option for extension developers to decide if an interruptible worker can be restarted or not, instead of always being thrown away. There can be an appeal in the restart if the bgworker relies on a database to connect to coming from a GUC, if its value can be reloaded. Identify the original worker by PID and backend start time in the test, so a restarted worker can reuse its PID without failing the termination check. Oversight in f1e251be80a0. Author: Zsolt Parragi Author: Michael Paquier Reviewed-by: Aya Iwata Reviewed-by: Xuneng Zhou Discussion: https://postgr.es/m/CAN4CZFNP+_aVsTqxfyNLU+AtRZMJeDRAR9XRfjPTwsWs3TXVaQ@mail.gmail.com Backpatch-through: 19 --- doc/src/sgml/bgworker.sgml | 6 ++-- src/backend/postmaster/bgworker.c | 31 ++++++++++-------- .../worker_spi/t/002_worker_terminate.pl | 32 +++++++++++++++++-- .../modules/worker_spi/worker_spi--1.0.sql | 3 +- src/test/modules/worker_spi/worker_spi.c | 4 ++- 5 files changed, 56 insertions(+), 20 deletions(-) diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml index 2affba7..f2627a8 100644 --- a/doc/src/sgml/bgworker.sgml +++ b/doc/src/sgml/bgworker.sgml @@ -115,8 +115,8 @@ typedef struct BackgroundWorker BGWORKER_INTERRUPTIBLE Requests termination of the background worker when its connected database is dropped, renamed, moved to a different tablespace, or used as a template for - CREATE DATABASE. Specifically, the postmaster sends a - termination signal when any of these commands affect the worker's database: + CREATE DATABASE. Specifically, a termination signal is + sent when any of these commands affect the worker's database: DROP DATABASE ALTER DATABASE RENAME @@ -125,6 +125,8 @@ typedef struct BackgroundWorker TABLESPACE CREATE DATABASE + The worker is then restarted or unregistered as after any other exit, + depending on its exit code and bgw_restart_time. Requires both BGWORKER_SHMEM_ACCESS and BGWORKER_BACKEND_DATABASE_CONNECTION. diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index 4aef037..e6948b9 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -1419,17 +1419,20 @@ GetBackgroundWorkerTypeByPid(pid_t pid) /* * Terminate all background workers connected to the given database, if the - * workers can be interrupted. + * workers can be interrupted. The workers are signaled directly. */ void TerminateBackgroundWorkersForDatabase(Oid databaseId) { - bool signal_postmaster = false; + pid_t *pids; + int npids = 0; elog(DEBUG1, "attempting worker termination for database %u", databaseId); - LWLockAcquire(BackgroundWorkerLock, LW_EXCLUSIVE); + pids = palloc_array(pid_t, BackgroundWorkerData->total_slots); + + LWLockAcquire(BackgroundWorkerLock, LW_SHARED); /* * Iterate through slots, looking for workers connected to the given @@ -1448,20 +1451,22 @@ TerminateBackgroundWorkersForDatabase(Oid databaseId) LWLockAcquire(ProcArrayLock, LW_SHARED); proc = BackendPidGetProcWithLock(pid); if (proc && proc->databaseId == databaseId) - { - slot->terminate = true; - signal_postmaster = true; - - elog(DEBUG1, "termination requested for worker (PID %d) on database %u", - (int) pid, databaseId); - } + pids[npids++] = pid; LWLockRelease(ProcArrayLock); } } LWLockRelease(BackgroundWorkerLock); - /* Make sure the postmaster notices the change to shared memory. */ - if (signal_postmaster) - SendPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE); + /* + * Signal the workers, avoiding kill() while holding the locks. + */ + for (int i = 0; i < npids; i++) + { + elog(DEBUG1, "termination requested for worker (PID %d) on database %u", + (int) pids[i], databaseId); + (void) kill(pids[i], SIGTERM); /* ignore any error */ + } + + pfree(pids); } diff --git a/src/test/modules/worker_spi/t/002_worker_terminate.pl b/src/test/modules/worker_spi/t/002_worker_terminate.pl index 69972a8..791bd60 100644 --- a/src/test/modules/worker_spi/t/002_worker_terminate.pl +++ b/src/test/modules/worker_spi/t/002_worker_terminate.pl @@ -19,12 +19,13 @@ if ($ENV{enable_injection_points} ne 'yes') # Returns the PID of the worker launched. sub launch_bgworker { - my ($node, $database, $testcase, $interruptible) = @_; + my ($node, $database, $testcase, $interruptible, $restart_time) = @_; + $restart_time //= -1; # Launch a background worker on the given database. my $pid = $node->safe_psql( 'postgres', qq( - SELECT worker_spi_launch($testcase, '$database'::regdatabase, 0, '{}', $interruptible); + SELECT worker_spi_launch($testcase, '$database'::regdatabase, 0, '{}', $interruptible, $restart_time); )); # Check that the bgworker is initialized and napping. @@ -42,6 +43,13 @@ sub launch_bgworker sub run_bgworker_interruptible_test { my ($node, $command, $testname, $pid) = @_; + + # A restarted worker may reuse the PID, so also record its start time. + my $backend_start = $node->safe_psql('postgres', + "SELECT backend_start FROM pg_stat_activity WHERE pid = $pid;"); + die "could not find start time for worker $pid" + unless length $backend_start; + my $offset = -s $node->logfile; $node->safe_psql('postgres', $command); @@ -56,7 +64,9 @@ sub run_bgworker_interruptible_test $offset); my $result = $node->safe_psql('postgres', - "SELECT count(*) = 0 FROM pg_stat_activity WHERE pid = $pid;"); + qq[SELECT count(*) = 0 FROM pg_stat_activity + WHERE pid = $pid + AND backend_start = '$backend_start'::timestamptz;]); is($result, 't', "dynamic bgworker stopped for $testname"); } @@ -161,4 +171,20 @@ run_bgworker_interruptible_test( "DROP DATABASE renameddb", "DROP DATABASE", $pid); +# Test case 5: restart after termination. Restart time at 0 implies zero +# delay in the restart. The old worker is known to have exited, so do not +# rely on the PID of the new worker being different, as it could be reused. +$pid = launch_bgworker($node, 'postgres', 5, 'true', 0); +run_bgworker_interruptible_test( + $node, + "CREATE DATABASE testdb2 WITH TEMPLATE postgres", + "CREATE DATABASE WITH TEMPLATE, restartable worker", $pid); +$result = $node->poll_query_until( + 'postgres', + qq[SELECT count(*) FROM pg_stat_activity + WHERE backend_type = 'worker_spi dynamic' + AND wait_event = 'WorkerSpiMain';], + '1'); +is($result, 1, "bgworker restarted after termination"); + done_testing(); diff --git a/src/test/modules/worker_spi/worker_spi--1.0.sql b/src/test/modules/worker_spi/worker_spi--1.0.sql index f5e9621..8d112a9 100644 --- a/src/test/modules/worker_spi/worker_spi--1.0.sql +++ b/src/test/modules/worker_spi/worker_spi--1.0.sql @@ -8,7 +8,8 @@ CREATE FUNCTION worker_spi_launch(index int4, dboid oid DEFAULT 0, roleoid oid DEFAULT 0, flags text[] DEFAULT '{}', - interruptible boolean DEFAULT false) + interruptible boolean DEFAULT false, + restart_time int4 DEFAULT -1) RETURNS pg_catalog.int4 STRICT AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c index b635a48..668c582 100644 --- a/src/test/modules/worker_spi/worker_spi.c +++ b/src/test/modules/worker_spi/worker_spi.c @@ -406,6 +406,7 @@ worker_spi_launch(PG_FUNCTION_ARGS) int nelems; Datum *datum_flags; bool interruptible = PG_GETARG_BOOL(4); + int restart_time = PG_GETARG_INT32(5); memset(&worker, 0, sizeof(worker)); worker.bgw_flags = BGWORKER_SHMEM_ACCESS | @@ -415,7 +416,8 @@ worker_spi_launch(PG_FUNCTION_ARGS) worker.bgw_flags |= BGWORKER_INTERRUPTIBLE; worker.bgw_start_time = BgWorkerStart_RecoveryFinished; - worker.bgw_restart_time = BGW_NEVER_RESTART; + worker.bgw_restart_time = restart_time >= 0 ? + restart_time : BGW_NEVER_RESTART; sprintf(worker.bgw_library_name, "worker_spi"); sprintf(worker.bgw_function_name, "worker_spi_main"); snprintf(worker.bgw_name, BGW_MAXLEN, "worker_spi dynamic worker %d", i); -- 2.50.1 (Apple Git-155)