public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aya Iwata (Fujitsu) <[email protected]>
To: 'Chao Li' <[email protected]>
To: Peter Smith <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
Date: Fri, 10 Oct 2025 09:04:54 +0000
Message-ID: <OS7PR01MB11964C077A3E61F4887DD3D09EAEFA@OS7PR01MB11964.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <[email protected]>
References: <OS7PR01MB119648846BA926C096CE016B6EAE3A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<CAHut+Pt4Tn1bQYCsYeUt_gtcSB-KOTtRB70SLghkpsjfKGsm7w@mail.gmail.com>
<OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<CAHut+Pu-nxNice547eGEW2O3hdRcFbPYWF4HiqktZO19X3Ah-g@mail.gmail.com>
<OS7PR01MB11964628A74081DFCA442CBADEAE1A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<CAHut+PuuOKKW2oDT0Z8q+UXsiteS67j6G6075FC3aAxeR0cxHQ@mail.gmail.com>
<OSCPR01MB14966FC50CFB457938763AE09F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<OSCPR01MB14966B98E5119EBB261024ED2F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB1496614832F8014EC16FB78D2F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB14966EC12277712131EB8EDF1F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<OS7PR01MB11964C8FE9CDCC0F4C9110988EAEEA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<[email protected]>
Hi,
Thank you for your comments. I updated patch to v0006.
> -----Original Message-----
> From: Peter Smith mailto:[email protected]
> Sent: Friday, October 10, 2025 7:57 AM
> FYI, I attached a v5 top-up diff for (some of) my above review
> comments in case it helps.
Thank you very much. I've updated the patch using the attached diff file.
> From: Chao Li <[email protected]>
> Sent: Friday, October 10, 2025 11:03 AM
> 1 - bgworker.sgml
> This paragraph has several English problems:
> * “Undergoes significant changes” sounds vague, better to say “is dropped, renamed or moved to a different tablespace”.
> * “When CREATE DATABASE TEMPLATE command is executed” - missing articles.
> * “background workers which connected to target template database” - wrong tense/relative pronoun.
> * “are not using” should be “are not used” or “are not set”
Thank you for your comment an suggested revision. I changed .sgml documentation.
>2 - bgworker.h
>```
>+#define BGWORKER_EXIT_AT_DATABASE_CHANGE 0x0004
>```
>
>You are using white-spaces between the macro name and value, that’s why 0x0004 looks not aligned in my IDE. I think you should use a couple tabs between them.
Thank you. I fixed this white-spaces (using pgindent).
>3 - bgworker.h
>```
>+extern void TerminateBackgroundWorkersByOid(Oid databaseId);
>```
>
> An OID can represent a lot of things. So, instead of suggesting the OID type by parameter name, I wonder if it is better do that with the function name, like TerminateBgWorkersByDbOid(Oid oid)
After receiving your comment, I checked other functions and there is no other examples like XXOid function in the code.
If this function use only here, original code is using databaseId in argument and it clear what Oid is.
I think original name is fine because it's not a function that's called much elsewhere.
> 4 - procarray.c
> ```
> + /*
> + * Terminate all background workers for this database, if
> + * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP)
> + */
> + TerminateBackgroundWorkersByOid(databaseId);
> ```
>
> I wonder if the correct parameter should be BGWORKER_EXIT_AT_DATABASE_CHANGE in the comment, as you are adding BGWORKER_EXIT_AT_DATABASE_CHANGE with this patch.
Thank you. I fixed this comment.
> 5 - bgworker.c
> ```
> +/*
> + * Cancel background workers.
> + */
> +void
> +TerminateBackgroundWorkersByOid(Oid databaseId)
> ```
>
> I think the function name is more descriptive than the function comment. So, please either remove function comment or enhance it.
I changed this function comment:
" Terminate all background workers connected to the given database, if they had requested it."
Regards,
Aya Iwata
Fujitsu Limited.
Attachments:
[application/octet-stream] v0006-0001-Allow-background-workers-to-be-terminated.patch (10.6K, 2-v0006-0001-Allow-background-workers-to-be-terminated.patch)
download | inline diff:
From 6567d1bf3b71b316f20382162dbd58f05f538ada Mon Sep 17 00:00:00 2001
From: "iwata.aya" <[email protected]>
Date: Thu, 11 Sep 2025 21:16:51 +0900
Subject: [PATCH v0006] Allow background workers to be terminated at DROP
DATABASE
---
doc/src/sgml/bgworker.sgml | 20 ++++
src/backend/postmaster/bgworker.c | 41 +++++++
src/backend/storage/ipc/procarray.c | 7 ++
src/include/postmaster/bgworker.h | 7 ++
src/test/modules/worker_spi/meson.build | 1 +
.../worker_spi/t/002_worker_terminate.pl | 106 ++++++++++++++++++
.../modules/worker_spi/worker_spi--1.0.sql | 3 +-
src/test/modules/worker_spi/worker_spi.c | 5 +
8 files changed, 189 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/worker_spi/t/002_worker_terminate.pl
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 2c393385a91..5a098e0c0ee 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -108,6 +108,26 @@ typedef struct BackgroundWorker
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>BGWORKER_EXIT_AT_DATABASE_CHANGE</literal></term>
+ <listitem>
+ <para>
+ <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</primary></indexterm>
+ Requests termination of the background worker when its connected database
+ is dropped, renamed, or moved to a different tablespace.
+ In these cases, the postmaster will send a termination signal to the
+ background worker when any of the following commands are executed:
+ <command>DROP DATABASE</command>,
+ <command>ALTER DATABASE RENAME TO</command>,
+ <command>ALTER DATABASE SET TABLESPACE</command>, or
+ <command>CREATE DATABASE</command> (when the worker is connected to the
+ template database).
+ This flag requires both <literal>BGWORKER_SHMEM_ACCESS</literal> and
+ <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 1ad65c237c3..ea1379ccc24 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"
@@ -1396,3 +1397,43 @@ GetBackgroundWorkerTypeByPid(pid_t pid)
return result;
}
+
+
+/*
+ * Terminate all background workers connected to the given database, if they
+ * had requested it.
+ */
+void
+TerminateBackgroundWorkersByOid(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_EXIT_AT_DATABASE_CHANGE))
+ {
+ 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 200f72c6e25..2d31c71fd8e 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -56,6 +56,7 @@
#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"
@@ -3768,6 +3769,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 had
+ * requested it (BGWORKER_EXIT_AT_DATABASE_CHANGE)
+ */
+ TerminateBackgroundWorkersByOid(databaseId);
+
/* sleep, then try again */
pg_usleep(100 * 1000L); /* 100ms */
}
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 058667a47a0..48eaf7af8d5 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -59,6 +59,12 @@
*/
#define BGWORKER_BACKEND_DATABASE_CONNECTION 0x0002
+/*
+ * Exit the bgworker when its database is dropped, renamed, or moved.
+ * No-op if BGWORKER_BACKEND_DATABASE_CONNECTION is not specified.
+ */
+#define BGWORKER_EXIT_AT_DATABASE_CHANGE 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 +134,7 @@ extern const char *GetBackgroundWorkerTypeByPid(pid_t pid);
/* Terminate a bgworker */
extern void TerminateBackgroundWorker(BackgroundWorkerHandle *handle);
+extern void TerminateBackgroundWorkersByOid(Oid databaseId);
/* This is valid in a running worker */
extern PGDLLIMPORT BackgroundWorker *MyBgworkerEntry;
diff --git a/src/test/modules/worker_spi/meson.build b/src/test/modules/worker_spi/meson.build
index d673ece48a0..1d30048aec8 100644
--- a/src/test/modules/worker_spi/meson.build
+++ b/src/test/modules/worker_spi/meson.build
@@ -28,6 +28,7 @@ tests += {
'tap': {
'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 00000000000..5d361c017d3
--- /dev/null
+++ b/src/test/modules/worker_spi/t/002_worker_terminate.pl
@@ -0,0 +1,106 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+
+# Test background workers can be terminated
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Ensure the worker_spi dynamic worker is launched on the specified database
+sub launch_bgworker
+{
+ my ($node, $database, $testcase, $allow_terminate) = @_;
+ my $offset = -s $node->logfile;
+
+ # Launch a background worker on the given database
+ my $result = $node->safe_psql(
+ $database, qq(
+ SELECT worker_spi_launch($testcase, oid, 0, '{}', $allow_terminate) IS NOT NULL
+ FROM pg_database WHERE datname = '$database';
+ ));
+ is($result, 't', "dynamic bgworker launched");
+
+ # Check the worker is surely initialized
+ $node->wait_for_log(
+ qr/LOG: worker_spi dynamic worker $testcase initialized with .*\..*/,
+ $offset);
+}
+
+# Run the given query and verify the background worker can be terminated
+sub run_db_command
+{
+ my ($node, $command, $testname) = @_;
+ my $offset = -s $node->logfile;
+
+ $node->safe_psql('postgres', $command);
+ ok( $node->log_contains(
+ "terminating background worker \"worker_spi dynamic\" due to administrator command",
+ $offset),
+ "background worker can be terminated at $testname");
+}
+
+my $node = PostgreSQL::Test::Cluster->new('mynode');
+$node->init;
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION worker_spi;');
+
+# Launch a background worker without BGWORKER_EXIT_AT_DATABASE_CHANGE
+launch_bgworker($node, 'postgres', 0, "false");
+
+# Ensure CREATE DATABASE WITH TEMPLATE fails because background worker retains
+#
+# XXX This spends more than 5 seconds because the backend retries counting
+# number of connecting processes 50 times. See CountOtherDBBackends().
+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");
+
+# Terminate the background worker for upcoming tests
+$node->safe_psql(
+ "postgres", qq(
+ SELECT pg_terminate_backend(pid)
+ FROM pg_stat_activity WHERE backend_type = 'worker_spi dynamic';));
+
+# Ensure BGWORKER_EXIT_AT_DATABASE_CHANGE allows background workers to be
+# terminated at some database manipulations.
+#
+# Testcase 1: CREATE DATABASE WITH TEMPLATE
+launch_bgworker($node, 'postgres', 1, "true");
+run_db_command(
+ $node,
+ "CREATE DATABASE testdb WITH TEMPLATE postgres",
+ "CREATE DATABASE WITH TEMPLATE");
+
+# Testcase 2: ALTER DATABASE RENAME
+launch_bgworker($node, 'testdb', 2, "true");
+run_db_command(
+ $node,
+ "ALTER DATABASE testdb RENAME TO renameddb",
+ "ALTER DATABASE RENAME");
+
+# Preparation for the next test; create another tablespace
+my $tablespace = PostgreSQL::Test::Utils::tempdir;
+$node->safe_psql('postgres',
+ "CREATE TABLESPACE test_tablespace LOCATION '$tablespace'");
+
+# Testcase 3: ALTER DATABASE SET TABLESPACE
+launch_bgworker($node, 'renameddb', 3, "true");
+run_db_command(
+ $node,
+ "ALTER DATABASE renameddb SET TABLESPACE test_tablespace",
+ "ALTER DATABASE SET TABLESPACE");
+
+# Testcase 4: DROP DATABASE
+launch_bgworker($node, 'renameddb', 4, "true");
+run_db_command($node, "DROP DATABASE renameddb", "DROP DATABASE");
+
+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 84deb6199f6..d29eee12d1f 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 '{}',
+ allow_termination 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 bea8339f464..22cce154d69 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 allow_termination = PG_GETARG_BOOL(4);
memset(&worker, 0, sizeof(worker));
worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
+
+ if (allow_termination)
+ worker.bgw_flags |= BGWORKER_EXIT_AT_DATABASE_CHANGE;
+
worker.bgw_start_time = BgWorkerStart_RecoveryFinished;
worker.bgw_restart_time = BGW_NEVER_RESTART;
sprintf(worker.bgw_library_name, "worker_spi");
--
2.39.3
view thread (67+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
In-Reply-To: <OS7PR01MB11964C077A3E61F4887DD3D09EAEFA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox