On Mon, Jan 05, 2026 at 01:44:55PM +1100, Peter Smith wrote: > It is not at all clear to me if you are advocating for the flag to be > called INTERRUPTABLE or to be called PROTECTED?
Actually, INTERRUPTABLE is still wrong. INTERRUPTIBLE reads more like correct English. > IIUC, behaviour-wise it ultimately ends up the same, just the flags > are different names with opposite meanings and defaults. Still, you > need to choose ASAP because this decision touches a lot of code, > comments and tests. Well, not exactly. I think that there is a pretty good argument in not breaking an existing behavior silently, which is what PROTECTED is about. > It seems that when Michael wrote, there were "more advantages in ... > make a bgworker interruptible an opt-in choice" [1], he is favouring > keeping it as the INTERRUPTABLE flag -- e.g. discard patch 0002. Am I > reading this thread correctly? You are reading that right: I do not see a point in 0002. The timing is interesting, I have put my hands on this patch this morning before you sent your last email, and adjusted the thing in many ways, finishing with the attached. This includes changes in the tests to address what I found was lacking and slightly incorrect, new names for the flag and its related variables, copyright update to 2026, as well as an additional sanity check when starting the workers, leading to the updated version attached. The CI is happy with it. Thoughts or comments about that? -- Michael
From 0ec550bdc60de4f2aaa5e81f8c7079d7818ee7dc Mon Sep 17 00:00:00 2001 From: "iwata.aya" <[email protected]> Date: Thu, 11 Sep 2025 21:16:51 +0900 Subject: [PATCH v12] Allow background workers to be terminated at DROP DATABASE --- src/include/postmaster/bgworker.h | 8 + src/backend/postmaster/bgworker.c | 51 ++++++ src/backend/storage/ipc/procarray.c | 28 +++- src/test/modules/worker_spi/Makefile | 4 + src/test/modules/worker_spi/meson.build | 4 + .../worker_spi/t/002_worker_terminate.pl | 148 ++++++++++++++++++ .../modules/worker_spi/worker_spi--1.0.sql | 3 +- src/test/modules/worker_spi/worker_spi.c | 5 + doc/src/sgml/bgworker.sgml | 19 +++ 9 files changed, 264 insertions(+), 6 deletions(-) create mode 100644 src/test/modules/worker_spi/t/002_worker_terminate.pl diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h index 5f84f48aa0d8..8acca22947d5 100644 --- a/src/include/postmaster/bgworker.h +++ b/src/include/postmaster/bgworker.h @@ -59,6 +59,13 @@ */ #define BGWORKER_BACKEND_DATABASE_CONNECTION 0x0002 +/* + * Exit the bgworker if its database is involved in a CREATE, ALTER or DROP + * database command. It requires BGWORKER_SHMEM_ACCESS and + * BGWORKER_BACKEND_DATABASE_CONNECTION. + */ +#define BGWORKER_INTERRUPTIBLE 0x0004 + /* * This class is used internally for parallel queries, to keep track of the * number of active parallel workers and make sure we never launch more than @@ -128,6 +135,7 @@ extern const char *GetBackgroundWorkerTypeByPid(pid_t pid); /* Terminate a bgworker */ extern void TerminateBackgroundWorker(BackgroundWorkerHandle *handle); +extern void TerminateInterruptibleBgWorkersByDbOid(Oid databaseId); /* This is valid in a running worker */ extern PGDLLIMPORT BackgroundWorker *MyBgworkerEntry; diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index af02eea754fa..6408510d6554 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -26,6 +26,7 @@ #include "storage/lwlock.h" #include "storage/pmsignal.h" #include "storage/proc.h" +#include "storage/procarray.h" #include "storage/procsignal.h" #include "storage/shmem.h" #include "tcop/tcopprot.h" @@ -665,6 +666,17 @@ SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel) /* XXX other checks? */ } + /* Interruptible workers require a database connection */ + if ((worker->bgw_flags & BGWORKER_INTERRUPTIBLE) && + !(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION)) + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("background worker \"%s\": cannot make background workers interruptible without database access", + worker->bgw_name))); + return false; + } + if ((worker->bgw_restart_time < 0 && worker->bgw_restart_time != BGW_NEVER_RESTART) || (worker->bgw_restart_time > USECS_PER_DAY / 1000)) @@ -1399,3 +1411,42 @@ GetBackgroundWorkerTypeByPid(pid_t pid) return result; } + +/* + * Terminate all background workers connected to the given database, if they + * had requested it. + */ +void +TerminateInterruptibleBgWorkersByDbOid(Oid databaseId) +{ + bool signal_postmaster = false; + + LWLockAcquire(BackgroundWorkerLock, LW_EXCLUSIVE); + + /* + * Iterate through slots, looking for workers connected to the given + * database. + */ + for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno) + { + BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno]; + + if (slot->in_use && + (slot->worker.bgw_flags & BGWORKER_INTERRUPTIBLE)) + { + PGPROC *proc = BackendPidGetProc(slot->pid); + + if (proc && proc->databaseId == databaseId) + { + slot->terminate = true; + signal_postmaster = true; + } + } + } + + LWLockRelease(BackgroundWorkerLock); + + /* Make sure the postmaster notices the change to shared memory. */ + if (signal_postmaster) + SendPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE); +} diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 52f886bf84fb..4c5efb7f9d20 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -56,11 +56,13 @@ #include "catalog/pg_authid.h" #include "miscadmin.h" #include "pgstat.h" +#include "postmaster/bgworker.h" #include "port/pg_lfind.h" #include "storage/proc.h" #include "storage/procarray.h" #include "utils/acl.h" #include "utils/builtins.h" +#include "utils/injection_point.h" #include "utils/lsyscache.h" #include "utils/rel.h" #include "utils/snapmgr.h" @@ -3687,8 +3689,9 @@ CountUserBackends(Oid roleid) * CountOtherDBBackends -- check for other backends running in the given DB * * If there are other backends in the DB, we will wait a maximum of 5 seconds - * for them to exit. Autovacuum backends are encouraged to exit early by - * sending them SIGTERM, but normal user backends are just waited for. + * for them to exit. Autovacuum backends and background workers are encouraged + * to exit early by sending them SIGTERM, but normal user backends are just + * waited for. * * The current backend is always ignored; it is caller's responsibility to * check whether the current backend uses the given DB, if it's important. @@ -3713,10 +3716,19 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared) #define MAXAUTOVACPIDS 10 /* max autovacs to SIGTERM per iteration */ int autovac_pids[MAXAUTOVACPIDS]; - int tries; - /* 50 tries with 100ms sleep between tries makes 5 sec total wait */ - for (tries = 0; tries < 50; tries++) + /* + * Retry up to 50 times with 100ms between attempts (max 5s total). Can be + * reduced to 3 attempts (max 0.3s total) to speed up tests. + */ + int ntries = 50; + +#ifdef USE_INJECTION_POINTS + if (IS_INJECTION_POINT_ATTACHED("reduce-ncounts")) + ntries = 3; +#endif + + for (int tries = 0; tries < ntries; tries++) { int nautovacs = 0; bool found = false; @@ -3766,6 +3778,12 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared) for (index = 0; index < nautovacs; index++) (void) kill(autovac_pids[index], SIGTERM); /* ignore any error */ + /* + * Terminate all background workers for this database, if they have + * requested it (BGWORKER_INTERRUPTIBLE). + */ + TerminateInterruptibleBgWorkersByDbOid(databaseId); + /* sleep, then try again */ pg_usleep(100 * 1000L); /* 100ms */ } diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile index 024b34cdbb35..e7c5c059e321 100644 --- a/src/test/modules/worker_spi/Makefile +++ b/src/test/modules/worker_spi/Makefile @@ -6,6 +6,10 @@ EXTENSION = worker_spi DATA = worker_spi--1.0.sql PGFILEDESC = "worker_spi - background worker example" +EXTRA_INSTALL = src/test/modules/injection_points + +export enable_injection_points + TAP_TESTS = 1 ifdef USE_PGXS diff --git a/src/test/modules/worker_spi/meson.build b/src/test/modules/worker_spi/meson.build index e9236bc27e71..6475e23f6017 100644 --- a/src/test/modules/worker_spi/meson.build +++ b/src/test/modules/worker_spi/meson.build @@ -26,8 +26,12 @@ tests += { 'sd': meson.current_source_dir(), 'bd': meson.current_build_dir(), 'tap': { + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, 'tests': [ 't/001_worker_spi.pl', + 't/002_worker_terminate.pl' ], }, } diff --git a/src/test/modules/worker_spi/t/002_worker_terminate.pl b/src/test/modules/worker_spi/t/002_worker_terminate.pl new file mode 100644 index 000000000000..2be3a122fe54 --- /dev/null +++ b/src/test/modules/worker_spi/t/002_worker_terminate.pl @@ -0,0 +1,148 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test background workers can be terminated by db commands + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# This test depends on injection points to detect whether background workers +# remain. +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +# Ensure the worker_spi dynamic worker is launched on the specified database. +# Returns the PID of the worker launched. +sub launch_bgworker +{ + my ($node, $database, $testcase, $request_terminate) = @_; + my $offset = -s $node->logfile; + + # Launch a background worker on the given database. + my $pid = $node->safe_psql( + $database, qq( + SELECT worker_spi_launch($testcase, '$database'::regdatabase, 0, '{}', $request_terminate); + )); + + # Check that the bgworker is initialized. + $node->wait_for_log( + qr/LOG: worker_spi dynamic worker $testcase initialized with .*\..*/, + $offset); + my $result = $node->safe_psql($database, + "SELECT count(*) > 0 FROM pg_stat_activity WHERE pid = $pid;"); + is($result, 't', "dynamic bgworker $testcase launched"); + + return $pid; +} + +# Run query and verify that the bgworker with the specified PID has been +# terminated. +sub run_db_command +{ + my ($node, $command, $testname, $pid) = @_; + my $offset = -s $node->logfile; + + $node->safe_psql('postgres', $command); + + $node->wait_for_log( + qr/terminating background worker \"worker_spi dynamic\" due to administrator command/, + $offset); + + my $result = $node->safe_psql('postgres', + "SELECT count(*) = 0 FROM pg_stat_activity WHERE pid = $pid;"); + is($result, 't', "dynamic bgworker stopped for $testname"); +} + +my $node = PostgreSQL::Test::Cluster->new('mynode'); +$node->init; +$node->start; + +# Check if the extension injection_points is available, as it may be +# possible that this script is run with installcheck, where the module +# would not be installed by default. +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$node->safe_psql('postgres', 'CREATE EXTENSION worker_spi;'); + +# Launch a background worker without BGWORKER_INTERRUPTIBLE. +my $pid = launch_bgworker($node, 'postgres', 0, 'false'); + +# Ensure CREATE DATABASE WITH TEMPLATE fails because a non-interruptible +# bgworker exists. + +# The injection point 'reduce-ncounts' reduces the number of backend +# retries, allowing for shorter test runs. See CountOtherDBBackends(). +$node->safe_psql('postgres', "CREATE EXTENSION injection_points;"); +$node->safe_psql('postgres', + "SELECT injection_points_attach('reduce-ncounts', 'error');"); + +my $stderr; + +$node->psql( + 'postgres', + "CREATE DATABASE testdb WITH TEMPLATE postgres", + stderr => \$stderr); +ok( $stderr =~ + "source database \"postgres\" is being accessed by other users", + "background worker blocked the database creation"); + +# Confirm that the non-interruptible bgworker is still running. +my $result = $node->safe_psql( + "postgres", qq( + SELECT count(1) FROM pg_stat_activity + WHERE backend_type = 'worker_spi dynamic';)); + +is($result, '1', + "background worker is still running after CREATE DATABASE WITH TEMPLATE"); + +# Terminate the non-interruptible worker for the next tests. +$node->safe_psql( + "postgres", qq( + SELECT pg_terminate_backend(pid) + FROM pg_stat_activity WHERE backend_type = 'worker_spi dynamic';)); + +# The injection point is not used anymore, release it. +$node->safe_psql('postgres', + "SELECT injection_points_detach('reduce-ncounts');"); + +# Check that BGWORKER_INTERRUPTIBLE allows background workers to be +# terminated with database-related commands. + +# Test case 1: CREATE DATABASE WITH TEMPLATE +$pid = launch_bgworker($node, 'postgres', 1, 'true'); +run_db_command( + $node, + "CREATE DATABASE testdb WITH TEMPLATE postgres", + "CREATE DATABASE WITH TEMPLATE", $pid); + +# Test case 2: ALTER DATABASE RENAME +$pid = launch_bgworker($node, 'testdb', 2, 'true'); +run_db_command( + $node, + "ALTER DATABASE testdb RENAME TO renameddb", + "ALTER DATABASE RENAME", $pid); + +# Preparation for the next test, create a tablespace. +my $tablespace = PostgreSQL::Test::Utils::tempdir; +$node->safe_psql('postgres', + "CREATE TABLESPACE test_tablespace LOCATION '$tablespace'"); + +# Test case 3: ALTER DATABASE SET TABLESPACE +$pid = launch_bgworker($node, 'renameddb', 3, 'true'); +run_db_command( + $node, + "ALTER DATABASE renameddb SET TABLESPACE test_tablespace", + "ALTER DATABASE SET TABLESPACE", $pid); + +# Test case 4: DROP DATABASE +$pid = launch_bgworker($node, 'renameddb', 4, 'true'); +run_db_command($node, "DROP DATABASE renameddb", "DROP DATABASE", $pid); + +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 84deb6199f63..f5e9621b0d1e 100644 --- a/src/test/modules/worker_spi/worker_spi--1.0.sql +++ b/src/test/modules/worker_spi/worker_spi--1.0.sql @@ -7,7 +7,8 @@ CREATE FUNCTION worker_spi_launch(index int4, dboid oid DEFAULT 0, roleoid oid DEFAULT 0, - flags text[] DEFAULT '{}') + flags text[] DEFAULT '{}', + interruptible boolean DEFAULT false) 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 7b0cd2e99b4a..d1e4a2bd9520 100644 --- a/src/test/modules/worker_spi/worker_spi.c +++ b/src/test/modules/worker_spi/worker_spi.c @@ -404,10 +404,15 @@ worker_spi_launch(PG_FUNCTION_ARGS) Size ndim; int nelems; Datum *datum_flags; + bool interruptible = PG_GETARG_BOOL(4); memset(&worker, 0, sizeof(worker)); worker.bgw_flags = BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION; + + if (interruptible) + worker.bgw_flags |= BGWORKER_INTERRUPTIBLE; + worker.bgw_start_time = BgWorkerStart_RecoveryFinished; worker.bgw_restart_time = BGW_NEVER_RESTART; sprintf(worker.bgw_library_name, "worker_spi"); diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml index 2c393385a91f..6c45aeafef6b 100644 --- a/doc/src/sgml/bgworker.sgml +++ b/doc/src/sgml/bgworker.sgml @@ -108,6 +108,25 @@ typedef struct BackgroundWorker </listitem> </varlistentry> + <varlistentry> + <term><literal>BGWORKER_INTERRUPTIBLE</literal></term> + <listitem> + <para> + <indexterm><primary>BGWORKER_INTERRUPTIBLE</primary></indexterm> + Requests termination of the background worker when its connected database is + dropped, renamed, moved to a different tablespace, or used as a template for + <command>CREATE DATABASE</command>. Specifically, the postmaster sends a + termination signal when any of these commands affect the worker's database: + <command>DROP DATABASE</command>, + <command>ALTER DATABASE RENAME TO</command>, + <command>ALTER DATABASE SET TABLESPACE</command>, or + <command>CREATE DATABASE</command>. + Requires both <literal>BGWORKER_SHMEM_ACCESS</literal> and + <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal>. + </para> + </listitem> + </varlistentry> + </variablelist> </para> -- 2.51.0
signature.asc
Description: PGP signature
