public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aya Iwata (Fujitsu) <[email protected]>
To: 'Peter Smith' <[email protected]>
To: 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: Tue, 7 Oct 2025 10:33:16 +0000
Message-ID: <OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@OS7PR01MB11964.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAHut+Pt4Tn1bQYCsYeUt_gtcSB-KOTtRB70SLghkpsjfKGsm7w@mail.gmail.com>
References: <OS7PR01MB11964335F36BE41021B62EAE8EAE4A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<OSCPR01MB149662AEA64F4E66F494C2584F5E3A@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OS7PR01MB119648846BA926C096CE016B6EAE3A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<CAHut+Pt4Tn1bQYCsYeUt_gtcSB-KOTtRB70SLghkpsjfKGsm7w@mail.gmail.com>
Hi,
Thank you for your comments. I updated patch to v0003.
> Do we still need the cancel_flags? I cannot find other reasons to terminate
> workers. Also the things I don't like is that
> BGWORKER_CANCEL_ADMIN_COMMANDS must
> have the same value as BGWORKER_EXIT_AT_DATABASE_DROP. Only one
> flag exists but
> it has 0x0004. Can we remove the argument and flags from the patch?
One reason for adding these flags was that I considered a case where
we might not want to allow all worker terminations during database deletion,
even when the BGWORKER_EXIT_AT_DATABASE_DROP flag is set.
However, This might be a rare case. Therefore, I removed these flags.
> Here are some more minor review comments:
>
> ======
> doc/src/sgml/bgworker.sgml
>
> 1. Typo?
>
> s/damon/daemon/
Thank you. Yes, it is a typo. I fixed this.
> ======
> src/backend/postmaster/bgworker.c
>
> 2.
> +void
> +CancelBackgroundWorkers(Oid databaseId, int cancel_flags)
> +{
> + int slotno;
> + bool signal_postmaster = false;
> +
> + LWLockAcquire(BackgroundWorkerLock, LW_EXCLUSIVE);
> +
> + for (slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)
> + {
> + BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno];
> +
> + /* Check worker slot. */
> + if (!slot->in_use)
> + continue;
> +
> + /* 1st, check cancel flags. */
> + if ((slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP) &
> cancel_flags)
> + {
> + PGPROC *proc = BackendPidGetProc(slot->pid);
> +
> + if (!proc)
> + continue;
> +
> + /* 2nd, compare databaseId. */
> + if (proc->databaseId == databaseId)
> + {
> + /*
> + * Set terminate flag in shared memory, unless slot has
> + * been reused.
> + */
> + slot->terminate = true;
> + signal_postmaster = true;
> + }
> + }
> + }
>
> 2a.
> Declare slotno as a 'for' loop variable.
Thank you. I fixed this.
> ~
>
> 2b.
> There seem to be excessive conditions in the code. Is it better to
> restructure with less, like:
>
> for (int slotno = 0; ...)
> {
> ...
>
> if (!slot->in_use)
> continue;
>
> if (slot flags are not set to drop)
> continue;
> proc = BackendPidGetProc(slot->pid);
> if (proc && proc->databaseId == databaseId)
> {
> slot->terminate = true;
> signal_postmaster = true;
> }
> }
Thank you. I fixed this, too.
Regards,
Aya Iwata
Fujitsu Limited
Attachments:
[application/octet-stream] v0003-0001-Allow-background-workers-to-be-terminated.patch (9.4K, 2-v0003-0001-Allow-background-workers-to-be-terminated.patch)
download | inline diff:
From d83e2dceda50e0dab1e90bdee14c1d3f5a90db7a Mon Sep 17 00:00:00 2001
From: "iwata.aya" <[email protected]>
Date: Thu, 11 Sep 2025 21:16:51 +0900
Subject: [PATCH v0003] Allow background workers to be terminated at DROP
DATABASE
---
doc/src/sgml/bgworker.sgml | 17 ++++
src/backend/postmaster/bgworker.c | 45 +++++++++++
src/backend/storage/ipc/procarray.c | 6 ++
src/include/postmaster/bgworker.h | 12 ++-
src/test/modules/worker_spi/meson.build | 1 +
.../worker_spi/t/002_worker_terminate.pl | 80 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 6 +-
7 files changed, 164 insertions(+), 3 deletions(-)
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..53bd0be408e 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -108,6 +108,23 @@ typedef struct BackgroundWorker
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>BGWORKER_EXIT_AT_DATABASE_DROP</literal></term>
+ <listitem>
+ <para>
+ <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_DROP</primary></indexterm>
+ Requests to terminate background worker when the database connected by
+ the background worker is changed. DBMS daemon can issue a termination
+ signal to the background worker.
+ This occurs only when significant changes affecting the entire database
+ take place.
+ Specifically, major changes include when the <command>DROP DATABASE</command>,
+ <command>ALTER DATABASE RENAME TO</command>, and
+ <command>ALTER DATABASE SET TABLESPACE</command> commands are executed.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 1ad65c237c3..a8657e9be63 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,47 @@ GetBackgroundWorkerTypeByPid(pid_t pid)
return result;
}
+
+
+/*
+ * Cancel background workers.
+ */
+void
+CancelBackgroundWorkers(Oid databaseId)
+{
+ bool signal_postmaster = false;
+
+ LWLockAcquire(BackgroundWorkerLock, LW_EXCLUSIVE);
+
+ for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)
+ {
+ BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno];
+
+ /* Check worker slot. */
+ if (!slot->in_use)
+ continue;
+
+ /* 1st, check cancel flags. */
+ if (slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP)
+ {
+ PGPROC *proc = BackendPidGetProc(slot->pid);
+
+ /* 2nd, compare databaseId. */
+ if (proc && proc->databaseId == databaseId)
+ {
+ /*
+ * Set terminate flag in shared memory, unless slot has
+ * been reused.
+ */
+ 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..7150f93d05d 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,11 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
for (index = 0; index < nautovacs; index++)
(void) kill(autovac_pids[index], SIGTERM); /* ignore any error */
+ /*
+ * Cancel background workers by admin commands.
+ */
+ CancelBackgroundWorkers(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..8354ab040d8 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -59,6 +59,14 @@
*/
#define BGWORKER_BACKEND_DATABASE_CONNECTION 0x0002
+/*
+ * This flag means the bgworker must be exit when the connecting database is
+ * being dropped or moved.
+ * It requires both BGWORKER_SHMEM_ACCESS and
+ * BGWORKER_BACKEND_DATABASE_CONNECTION were passed too.
+ */
+#define BGWORKER_EXIT_AT_DATABASE_DROP 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
@@ -68,7 +76,6 @@
#define BGWORKER_CLASS_PARALLEL 0x0010
/* add additional bgworker classes here */
-
typedef void (*bgworker_main_type) (Datum main_arg);
/*
@@ -161,4 +168,7 @@ extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, ui
extern void BackgroundWorkerBlockSignals(void);
extern void BackgroundWorkerUnblockSignals(void);
+/* Cancel background workers. */
+extern void CancelBackgroundWorkers(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..78dcd40ae34
--- /dev/null
+++ b/src/test/modules/worker_spi/t/002_worker_terminate.pl
@@ -0,0 +1,80 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+
+# Test background workers can be terminated when ALTER/DROP DATABASE happens
+
+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) = @_;
+
+ # Launch a background worker on the given database
+ my $result = $node->safe_psql(
+ $database, qq(
+ SELECT worker_spi_launch(4, oid) IS NOT NULL
+ FROM pg_database WHERE datname = '$database';
+ ));
+ is($result, 't', "dynamic bgworker launched");
+
+ # Check the worker is running correctly
+ $result = $node->safe_psql('postgres',
+ "SELECT count(1) FROM pg_stat_activity WHERE datname = '$database' AND backend_type = 'worker_spi dynamic';"
+ );
+ is($result, 1, 'dynamic bgworker has exist');
+}
+
+# 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;
+
+# Create another database. "postgres" is still used to run db commands and
+# checks the pg_stat_activity.
+$node->safe_psql('postgres', 'CREATE DATABASE testdb');
+$node->safe_psql('testdb', 'CREATE EXTENSION worker_spi;');
+
+launch_bgworker($node, 'testdb');
+
+# Testcase 1: ALTER DATABASE SET TABLESPACE
+run_db_command(
+ $node,
+ "ALTER DATABASE testdb RENAME TO renameddb",
+ "ALTER DATABASE SET TABLESPACE");
+
+# Preparation for the next test; create another tablespace
+my $basedir = $node->basedir();
+my $tablespace = "$basedir/tablespace";
+mkdir($tablespace);
+$node->safe_psql('postgres',
+ "CREATE TABLESPACE test_tablespace LOCATION '$tablespace'");
+launch_bgworker($node, 'renameddb');
+
+# Testcase 2: ALTER DATABASE SET TABLESPACE
+run_db_command(
+ $node,
+ "ALTER DATABASE renameddb SET TABLESPACE test_tablespace",
+ "ALTER DATABASE SET TABLESPACE");
+
+launch_bgworker($node, 'renameddb');
+
+# Testcase 2: DROP DATABASE
+run_db_command($node, "DROP DATABASE renameddb", "DROP DATABASE");
+
+done_testing();
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index bea8339f464..dea9fd8281a 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -361,7 +361,8 @@ _PG_init(void)
/* set up common data for all our workers */
memset(&worker, 0, sizeof(worker));
worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
- BGWORKER_BACKEND_DATABASE_CONNECTION;
+ BGWORKER_BACKEND_DATABASE_CONNECTION |
+ BGWORKER_EXIT_AT_DATABASE_DROP;
worker.bgw_start_time = BgWorkerStart_RecoveryFinished;
worker.bgw_restart_time = BGW_NEVER_RESTART;
sprintf(worker.bgw_library_name, "worker_spi");
@@ -407,7 +408,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;
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: <OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@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