public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aya Iwata (Fujitsu) <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Michael Paquier' <[email protected]>
To: 'Peter Smith' <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
Date: Thu, 9 Oct 2025 13:09:23 +0000
Message-ID: <OS7PR01MB11964C8FE9CDCC0F4C9110988EAEEA@OS7PR01MB11964.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <OSCPR01MB14966EC12277712131EB8EDF1F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
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>
Hi,
I updated the patch to v0005.
> -----Original Message-----
> From: Peter Smith <[email protected]>
> Sent: Thursday, October 9, 2025 7:18 AM
> src/backend/postmaster/bgworker.c
>
> 1.
> 1a.
> It's not clear to me what you were trying to convey by saying "unless
> slot has been used" in the comment. Maybe you meant "unless slot is
> not in use", but is that useful even to say? Anyway, the comment as-is
> seems incorrect.
I changed this comment. I use Kuroda-san's comment. Thank you. )
"Iterate through slots, looking for workers who connects to the given database."
> 1b.
> Sorry for wavering on this, but now that I see the resulting v4 code,
> I feel we don't really need any of those 'continues', and more if
> conditions can be combined. It becomes simpler. See if you agree.
I changed if condition code.
> src/backend/storage/ipc/procarray.c
>
> 2.
> I was wondering about this function name "CancelXXX" -- do you
> "cancel" a worker, or do you "terminate" it?
>
> Isn't it better to name this new function more like the
> existing/similar TerminateBackgroundWorker() function?
>
> E.g. consider the following:
>
> /*
> * Terminate all background workers for this database, if
> * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP).
> */
> TerminateBackgroundWorkersForDB(databaseId);
I changed this name to "TerminateBackgroudWorkerByOid"
> -----Original Message-----
> From: Michael Paquier <[email protected]>
> Sent: Thursday, October 9, 2025 12:34 PM
> > Sorry for posting many times. I noticed that CountOtherDBBackends() can be
> called
> > while creating the database. Should we also mention and test the case?
>
> How would you test that? A bgworker would not be able to connect to
> the database that's being created.
> +my $basedir = $node->basedir();
> +my $tablespace = "$basedir/tablespace";
>
> We could use a temporary folder for the tablespaces. I've always
> prefered this practice. That's a bit, feel free to ignore this one,
> what you are doing is not wrong, either.
I use tempdir to create directory for the tablespace.
> +
> <term><literal>BGWORKER_EXIT_AT_DATABASE_DROP</literal></term>
>
> Perhaps BGWORKER_EXIT_AT_DATABASE_CHANGE? DROP is incorrect, as
> the
> database could be renamed or moved, as well.
>
> + * Exit the bgworker when its database is dropped, renamed, or moved.
> + * Requires BGWORKER_SHMEM_ACCESS and
> BGWORKER_BACKEND_DATABASE_CONNECTION.
> + */
> +#define BGWORKER_EXIT_AT_DATABASE_DROP
> 0x0004
>
> We could enforce this rule with an elog(ERROR) or an assert, perhaps?
I started think it does not a strict condition.
If the worker does not connect to databases,
the BGWORKER_EXIT_AT_DATABASE_DROP(CHANGE) flag will be never checked.
Therefore, I have changed this comment as follows:
" No-op if BGWORKER_BACKEND_DATABASE_CONNECTION are not specified."
> @@ -407,7 +407,8 @@ worker_spi_launch(PG_FUNCTION_ARGS)
> memset(&worker, 0, sizeof(worker));
> worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
> - BGWORKER_BACKEND_DATABASE_CONNECTION;
> + BGWORKER_BACKEND_DATABASE_CONNECTION |
> + BGWORKER_EXIT_AT_DATABASE_DROP;
>
> I would suggest to make this part optional, with an argument that
> uses a default value to false (as in "do-not-set the flag") that can
> be given by the callers of worker_spi_launch(). Let's also add one
> bgworker that's used in your series of tests, and check that it is
> *not* cancelled when the flag is not set.
I fixed this to added allow_termination flag and *not* canceled test.
However this test takes at least 5 sec. because of "for loop" in CountOtherDBBackends().
> +# Ensure the worker_spi dynamic worker is launched on the specified
> database
> +sub launch_bgworker
> +{
> + my ($node, $database) = @_;
> +
> + # Launch a background worker on the given database
> + my $result = $node->safe_psql(
> + $database, qq(
> + SELECT worker_spi_launch(4, oid) IS NOT NULL
>
> I'd recommend to make the worker number an argument of this function,
> and also do things so as the log_contains() call is able to check that
> the worker with the matching number is loged, rather than rely on
> "worker_spi dynamic" for all the comparisons. This is relevant to be
> able to mix multiple workers at the same time in the tests.
I fixed test code. Thank you for your advice.
It is better to used wait_for_log because it takes time for the log to output.
And we added test for "CREATE DATABASE TEMPLATE " command too.
Regards,
Aya Iwata
Fujitsu Limited
Attachments:
[application/octet-stream] v0005-0001-Allow-background-workers-to-be-terminated.patch (10.5K, 2-v0005-0001-Allow-background-workers-to-be-terminated.patch)
download | inline diff:
From 52040754e974fe2adad7966b1ecca3f44957556b Mon Sep 17 00:00:00 2001
From: "iwata.aya" <[email protected]>
Date: Thu, 11 Sep 2025 21:16:51 +0900
Subject: [PATCH v0005] Allow background workers to be terminated at DROP
DATABASE
---
doc/src/sgml/bgworker.sgml | 20 ++++
src/backend/postmaster/bgworker.c | 39 +++++++
src/backend/storage/ipc/procarray.c | 7 ++
src/include/postmaster/bgworker.h | 9 ++
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..01ceb3864e7 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 the database it is
+ connected to undergoes significant changes. 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>, or
+ <command>ALTER DATABASE SET TABLESPACE</command>.
+ When <command>CREATE DATABASE TEMPLATE</command> command is executed,
+ background workers which connected to target template database are terminated.
+ If <literal>BGWORKER_SHMEM_ACCESS</literal> and
+ <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal> are not using,
+ nothing happens.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 1ad65c237c3..7ab48c245ea 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,41 @@ GetBackgroundWorkerTypeByPid(pid_t pid)
return result;
}
+
+
+/*
+ * Cancel background workers.
+ */
+void
+TerminateBackgroundWorkersByOid(Oid databaseId)
+{
+ bool signal_postmaster = false;
+
+ LWLockAcquire(BackgroundWorkerLock, LW_EXCLUSIVE);
+
+ /*
+ * Iterate through slots, looking for workers
+ * who connects 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..f612e776dbf 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_DROP)
+ */
+ 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..57741d74987 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
@@ -161,4 +167,7 @@ extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, ui
extern void BackgroundWorkerBlockSignals(void);
extern void BackgroundWorkerUnblockSignals(void);
+/* Cancel background workers. */
+extern void TerminateBackgroundWorkersByOid(Oid databaseId);
+
#endif /* BGWORKER_H */
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..e0d0e8ab861 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]
Subject: RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
In-Reply-To: <OS7PR01MB11964C8FE9CDCC0F4C9110988EAEEA@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