On Mon, Nov 09, 2026 at 10:39:37AM +0800, Xuneng Zhou wrote: > That's what I've in mind. My original thought is to add worker > selection and terminatiion into the existing CountOtherDBBackends > scan. That avoids the separate worker-slot scan and PID lookups in > ForDatabase(), at the cost of adding and initializing a new marker in > PGPROC like PROC_IS_BGWORKER_INTERRUPTIBLE. Is this sensible/doable?
Hmm, no. I had something less complicated in mind, as of the patch attached. Note also that I have changed the SQL function to use a integer for the restart time, defaulting to -1 to map with the never-restart. Passing down a restart time of 0 also makes the test faster, as the worker is restarted without a delay. So the attached patch removes the need for a flag, and just relies on a plain kill(). Once we do that, the restart time gets effective when set. -- Michael
From 6179979428762d62103aa96371db8b405c2af3e7 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 9 Nov 2026 12:31:49 +0900 Subject: [PATCH v2] 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 nor, 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. Oversight in f1e251be80a0. Author: Zsolt Parragi <[email protected]> Author: Michael Paquier <[email protected]> Reviewed-by: Aya Iwata <[email protected]> Reviewed-by: Xuneng Zhou <[email protected]> Discussion: https://postgr.es/m/can4czfnp+_avstqxfynlu+atrzmjedrar9xrfjptwsws3tx...@mail.gmail.com Backpatch-through: 19 --- src/backend/postmaster/bgworker.c | 33 +++++++++++-------- .../worker_spi/t/002_worker_terminate.pl | 20 +++++++++-- .../modules/worker_spi/worker_spi--1.0.sql | 3 +- src/test/modules/worker_spi/worker_spi.c | 4 ++- doc/src/sgml/bgworker.sgml | 2 ++ 5 files changed, 44 insertions(+), 18 deletions(-) diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index 4aef037d5aed..c08f049aba54 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -1419,23 +1419,26 @@ 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, max_worker_processes); + + LWLockAcquire(BackgroundWorkerLock, LW_SHARED); /* * Iterate through slots, looking for workers connected to the given * database. */ - for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; slotno++) + for (int slotno = 0; slotno < max_worker_processes; slotno++) { BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno]; @@ -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 69972a838dc4..234fff75fb94 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. @@ -161,4 +162,19 @@ 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. +$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 pid <> $pid + 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 f5e9621b0d1e..8d112a943271 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 b635a4863494..668c582de434 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); diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml index 2affba743825..0d4eeb3869a1 100644 --- a/doc/src/sgml/bgworker.sgml +++ b/doc/src/sgml/bgworker.sgml @@ -125,6 +125,8 @@ typedef struct BackgroundWorker TABLESPACE</command></para></listitem> <listitem><para><command>CREATE DATABASE</command></para></listitem> </itemizedlist> + The worker is then restarted based on + <structfield>bgw_restart_time</structfield>, if set. Requires both <literal>BGWORKER_SHMEM_ACCESS</literal> and <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal>. </para> -- 2.56.0
signature.asc
Description: PGP signature
