Обсуждение: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE

Dear Iwata-san,

>Background
>==========
>If the background workers connect to databases, some database-related commands
>like ALTER DATABASE RENAME and ALTER DATABASE SET TABLESPACE cannot be done.
>Users must do DROP EXTENSION related with workers, or terminate them by themselves
>if they want to drop or alter the database.
>
>Proposal
>========
>Based on above, I would like to propose to terminate background workers automatically
>when such SQLs are executed.
 >
>This feature allows the DBMS daemon to send a termination signal to background workers
>created by users currently operating on the database when executing commands that make
>significant changes to the database.

Per my understanding, we already have a facility that terminates a background
worker, TerminateBackgroundWorker(). So, I'm afraid your proposal has already
been done by combining this function and ProcessUtility_hook.

So, is the main benefit of the patch to shorten extensions codes which uses
bgworker?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Oct 06, 2025 at 01:59:08AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Per my understanding, we already have a facility that terminates a background
> worker, TerminateBackgroundWorker(). So, I'm afraid your proposal has already
> been done by combining this function and ProcessUtility_hook.

The main take that I am getting here from Iwata-san is that this would
lead to less code duplication.  Another item, which you are not
mentioning, is that this would be more flexible with bgworkers that
have been starting dynamically, where shared_preload_libraries may not
be used, still a bgworker would need to react.  So the suggestion of a
new API to control if a bgworker should be stopped like any other
backend when there is a database activity worth it is a good one, as
long as it is in line with what we do with normal backends.

AcceptBackgroundWorkerCancel() is going backwards, IMO.  Wouldn't it
be better to pass it down as an option in bgw_flags, to mark that a
bgworker connected to a database can be shutdown due to the effect of
a database getting dropped or moved?  There could be an argument
behind using bgw_extra, but that would not be in line with the
database connection argument which is a state defined when a bgworker
is registered.

> So, is the main benefit of the patch to shorten extensions codes which uses
> bgworker?

I'd ask for the addition of tests when it comes to such a facility,
and your proposal has none of that.  I would suggest worker_spi with
an option that can be passed to worker_spi_launch().
--
Michael

Dear Michael,

> The main take that I am getting here from Iwata-san is that this would
> lead to less code duplication.  Another item, which you are not
> mentioning, is that this would be more flexible with bgworkers that
> have been starting dynamically, where shared_preload_libraries may not
> be used, still a bgworker would need to react.  So the suggestion of a
> new API to control if a bgworker should be stopped like any other
> backend when there is a database activity worth it is a good one, as
> long as it is in line with what we do with normal backends.

Okay, so this proposal has the advantage that we can terminate workers, even if the
extensions do not control workers on the shared memory, right?

> AcceptBackgroundWorkerCancel() is going backwards, IMO.  Wouldn't it
> be better to pass it down as an option in bgw_flags, to mark that a
> bgworker connected to a database can be shutdown due to the effect of
> a database getting dropped or moved?

+1 to extend the bgw_flags.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

Thank you for your comments.

> On Mon, Oct 06, 2025 at 01:59:08AM +0000, Hayato Kuroda (Fujitsu) wrote:
> > Per my understanding, we already have a facility that terminates a
> background
> > worker, TerminateBackgroundWorker(). So, I'm afraid your proposal has
> already
> > been done by combining this function and ProcessUtility_hook.
>
> The main take that I am getting here from Iwata-san is that this would
> lead to less code duplication.

Yeah. My company sometimes wrote extensions which uses background workers,
and it was painful to implement the same part every time.

> lead to less code duplication.  Another item, which you are not
> mentioning, is that this would be more flexible with bgworkers that
> have been starting dynamically, where shared_preload_libraries may not
> be used, still a bgworker would need to react.  So the suggestion of a
> new API to control if a bgworker should be stopped like any other
> backend when there is a database activity worth it is a good one, as
> long as it is in line with what we do with normal backends.

Thanks for the agreement. Thus I want to proceed the patch.

> AcceptBackgroundWorkerCancel() is going backwards, IMO.  Wouldn't it
> be better to pass it down as an option in bgw_flags, to mark that a
> bgworker connected to a database can be shutdown due to the effect of
> a database getting dropped or moved?  There could be an argument
> behind using bgw_extra, but that would not be in line with the
> database connection argument which is a state defined when a bgworker
> is registered.

I updated my patch using bgw_flags to set whether accept to terminate bgworker or not.
And I also removed AcceptBackgroundWorkerCancel() function.
Please check my attached patch.

>
> > So, is the main benefit of the patch to shorten extensions codes which uses
> > bgworker?
>
> I'd ask for the addition of tests when it comes to such a facility,
> and your proposal has none of that.  I would suggest worker_spi with
> an option that can be passed to worker_spi_launch().

I added the TAP test using worker_spi too.

Regards,
Aya Iwata
Fujitsu Limited

Dear Iwata-san,

Thanks for updating the patch.

> I updated my patch using bgw_flags to set whether accept to terminate bgworker
> or not.
> And I also removed AcceptBackgroundWorkerCancel() function.
> Please check my attached patch.

```
+/*
+ * Cancel background workers.
+ */
+void
+CancelBackgroundWorkers(Oid databaseId, int cancel_flags)
```

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?

[1]:
https://www.postgresql.org/message-id/OSCPR01MB149662AEA64F4E66F494C2584F5E3A%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

Here are some more minor review comments:

======
doc/src/sgml/bgworker.sgml

1. Typo?

s/damon/daemon/

======
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.

~

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;
  }
}

======
Kind Regards,
Peter Smith.
Fujitsu Australia

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

Dear Iwata-san,

Thanks for updating the patch. Comments:

```
+        /* Check worker slot. */
+        if (!slot->in_use)
+            continue;
```

The comment has less meaning. How about:
"Skip if the slot is not used"

```
+        /* 1st, check cancel flags. */
+        if (slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP)
```

Missing update 2b [1]. Also, since cancel flag does not exist anymore, the comment should be
Updated. How about something like:
"Skip if the background worker does not want to exit"

```
+            /* 2nd, compare databaseId. */
+            if (proc && proc->databaseId == databaseId)
```

Here should describes what are you trying to do. How about something like:
Checks the connecting database of the worker, and instruct the postmaster to terminate it if needed

```
+        /*
+         * Cancel background workers by admin commands.
+         */
+        CancelBackgroundWorkers(databaseId);
```

Since we removed the flag, the comment is outdated.

```
-
 typedef void (*bgworker_main_type) (Datum main_arg);
```

This change is not related with this patch.

```
@@ -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;
```

The new flag was added to both static and dynamic background workers. So, how about
testing both? I think it is enough to use one of case, like ALTER DATABASE SET TABLESPACE.

[1]: https://www.postgresql.org/message-id/CAHut%2BPt4Tn1bQYCsYeUt_gtcSB-KOTtRB70SLghkpsjfKGsm7w%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

HI Iwata-San,

Here are some more review comments for v3.

======
doc/src/sgml/bgworker.sgml

1.
+      <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>

Here is a reworded version of that for your consideration
(AI-generated -- pls verify for correctness!):

<para>
 <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_DROP</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>.
</para>

======


2.
+ 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;
+ }
+ }
+ }


IMO, most of those comments do not have any benefit because they only
repeat what is already obvious from the code.

2a.
+ /* Check worker slot. */
+ if (!slot->in_use)
Remove that one. It is the same as the code.

~

2b.
+ /* 1st, check cancel flags. */
+ if (slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP)
Remove that one. It is the same as the code

~

2c.
+ /* 2nd, compare databaseId. */
+ if (proc && proc->databaseId == databaseId)
Remove that one. It is the same as the code.

~

2d.
+ /*
+ * Set terminate flag in shared memory, unless slot has
+ * been reused.
+ */

This comment is a bit strange -- It seems slightly misplaced. IIUC,
the "unless slot has been reused" really is referring to the earlier
"slot->in_use". This whole comment may be better put immediately above
the 'for' loop as a short summary of the whole logic.

======
src/include/postmaster/bgworker.h

3.
+/*
+ * 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.
+ */

Not English. Needs rewording. Consider something like this:

/*
 * Exit the bgworker when its database is dropped, renamed, or moved.
 * Requires BGWORKER_SHMEM_ACCESS and BGWORKER_BACKEND_DATABASE_CONNECTION.
 */

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Hi Kuroda-san, Peter-san,

Thank you for your review comments! I updated patch to v0004.

> -----Original Message-----
> From: Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com>
> Sent: Tuesday, October 7, 2025 8:49 PM

> ```
> +            /* 2nd, compare databaseId. */
> +            if (proc && proc->databaseId == databaseId)
> ```
> 
> Here should describes what are you trying to do. How about something like:
> Checks the connecting database of the worker, and instruct the postmaster to
> terminate it if needed

This comment has been removed. We can know code's intent without comments.

> ```
> +        /*
> +         * Cancel background workers by admin commands.
> +         */
> +        CancelBackgroundWorkers(databaseId);
> ```
> 
> Since we removed the flag, the comment is outdated.

Thank you. I changed this comment:
"if set the bgw_flags, cancel background workers."

> ```
> -
>  typedef void (*bgworker_main_type) (Datum main_arg);
> ```
> 
> This change is not related with this patch.

I removed this change.

> ```
> @@ -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;
> ```
> 
> The new flag was added to both static and dynamic background workers. So,
> how about
> testing both? I think it is enough to use one of case, like ALTER DATABASE SET
> TABLESPACE.

I removed this BGWORKER_EXIT_AT_DATABASE_DROP from the patch.

> -----Original Message-----
> From: Peter Smith <smithpb2250@gmail.com>

> ======
> doc/src/sgml/bgworker.sgml
> 
> 1.


> Here is a reworded version of that for your consideration
> (AI-generated -- pls verify for correctness!):
> 
> <para>
> 
> <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_DROP</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>.
> </para>

Thank you. I checked and replaced the doc.

> ======
> 
> 
> 2.
 
> IMO, most of those comments do not have any benefit because they only
> repeat what is already obvious from the code.
> 
> 2a.
> + /* Check worker slot. */
> + if (!slot->in_use)
> Remove that one. It is the same as the code.

I removed this comment.

> 2b.
> + /* 1st, check cancel flags. */
> + if (slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP)
> Remove that one. It is the same as the code

I removed this comment, too.

> 2c.
> + /* 2nd, compare databaseId. */
> + if (proc && proc->databaseId == databaseId)
> Remove that one. It is the same as the code.

I removed this comment, too.

> 2d.
> + /*
> + * Set terminate flag in shared memory, unless slot has
> + * been reused.
> + */
> 
> This comment is a bit strange -- It seems slightly misplaced. IIUC,
> the "unless slot has been reused" really is referring to the earlier
> "slot->in_use". This whole comment may be better put immediately above
> the 'for' loop as a short summary of the whole logic.

I put this comment to before the for 'loop'.
And I changed comment like this:

/*
 * Set terminate flag in shared memory, unless slot has
 * been used.
 */

> ======
> src/include/postmaster/bgworker.h
> 
> 3.
> +/*
> + * 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.
> + */
> 
> Not English. Needs rewording. Consider something like this:
> 
> /*
>  * Exit the bgworker when its database is dropped, renamed, or moved.
>  * Requires BGWORKER_SHMEM_ACCESS and
> BGWORKER_BACKEND_DATABASE_CONNECTION.
>  */

Thank you. I changed the source code comments based on this comment.
 
Regards,
Aya Iwata
Fujitsu Limited

Hi Iwata-San,

Some v4 comments.

======
src/backend/postmaster/bgworker.c

1.
+ /*
+ * Set terminate flag in shared memory, unless slot has
+ * been used.
+ */
+ for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)
+ {
+ PGPROC     *proc;
+ BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno];
+
+ if (!slot->in_use)
+ continue;
+
+ if (!(slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP))
+ continue;
+
+ proc = BackendPidGetProc(slot->pid);
+
+ if (proc && proc->databaseId == databaseId)
+ {
+ slot->terminate = true;
+ signal_postmaster = true;
+ }
+ }

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.

~

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.

SUGGESTION:

for (int slotno ...)
{
  if (slot->in_use && (slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP))
  {
    PGPROC *proc = BackendPidGetProc(slot->pid);
    if (proc && proc->databaseId == databaseId)
    {
      slot->terminate = true;
      signal_postmaster = true;
    }
  }
}

======
src/backend/storage/ipc/procarray.c

2.
+ /*
+ * if set the bgw_flags, cancel background workers.
+ */
+ CancelBackgroundWorkers(databaseId);
+

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);


======
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Peter, Iwata-san,

> 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.

Agreed to update the comment. How about:
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.

Ether way is fine for me.

> /*
>  * Terminate all background workers for this database, if
>  * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP).
>  */
> TerminateBackgroundWorkersForDB(databaseId);

The code comment looks OK. Regarding the function name, I want to propose
an alternative - TerminateBackgroundWorkersByOid().
Core codes have already had several xxxByOid() functions.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Iwata-san,

Sorry for posting many times. I noticed that CountOtherDBBackends() can be called
while creating the database. Should we also mention and test the case?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Oct 09, 2025 at 02:28:56AM +0000, Hayato Kuroda (Fujitsu) wrote:
> 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.

The implementation done in v4 is a nice simplification compared to the
original proposal, nice.  I've gone through the patch and here are
some comments.

+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.

+     <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?

@@ -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.

+# 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.
--
Michael

Dear Michael,

> > 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.
>

Sorry for missing words. Per my understanding, CountOtherDBBackends() in createdb()
ensures that there are no active connections of the source database. If there is
a connection to a database, we cannot create another database with TEMPALATE clause:

```
postgres=# CREATE DATABASE new TEMPLATE postgres ;
ERROR:  source database "postgres" is being accessed by other users
DETAIL:  There is 1 other session using the database.
```

Based on that, I imagined that we could launch a bgworker and create another database
by using template. Or is it already handled?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Oct 09, 2025 at 04:00:24AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Based on that, I imagined that we could launch a bgworker and create
> another database by using template. Or is it already handled?

Oh, I was not following you here.  Yes, we should have something with
a bgworker connected to a source and make sure that it disconnects
when create a new database with this source as template.

However, could there be more to consider here?  Contrary to DROP
DATABASE, where we require the drop to be done by the owner of the
database (or a superuser), CREATE DATABASE has less requirements: it
is fine for a role to create a database if they have the CREATEDB
rights.  If we allow bgworkers to be cancelled when the database they
are connected to is used as a source, that may be disruptive, so we
had better document precisely the behavior of the flag and what users
should expect from it when set.  We are already killing autovacuum
workers when they are connected to a database used as a source anyway
(since 4abd7b49f1e9 from 2008), so I am worrying for nothing and it is
actually better to apply the same rule across the board.  Let's just
document very clearly how the bgworker will react when the cancel flag
is set in all these cases.
--
Michael

Dear Michael,

> However, could there be more to consider here?  Contrary to DROP
> DATABASE, where we require the drop to be done by the owner of the
> database (or a superuser), CREATE DATABASE has less requirements: it
> is fine for a role to create a database if they have the CREATEDB
> rights. If we allow bgworkers to be cancelled when the database they
> are connected to is used as a source, that may be disruptive, so we
> had better document precisely the behavior of the flag and what users
> should expect from it when set.

Actually, if the database is not marked as the template one, the user must be
owner of the source or superuser. Not sure there is a real case that template
database has dedicated workers, but anyway I do agree to note down this behavior.
It is surprising that creating other databases lead the process terminations.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

I updated the patch to v0005.

> -----Original Message-----
> From: Peter Smith <smithpb2250@gmail.com>
> 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 <michael@paquier.xyz>
> 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

Hi Iwata-San,

Some v5 comments.

======
doc/src/sgml/bgworker.sgml

1.
+    <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>
+

1a.
The newly added part in v5 needs some brush-up.

SUGGESTION:

<para>
 <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</primary></indexterm>
 Requests termination of the background worker when its connected database
 undergoes significant changes. 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> (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>

~

1b.
Elsewhere on this docs page, there are detailed discussions about
process termination (e.g. search "terminate"), as well as referring to
the function TerminateBackgroundWorker().

You only documented the new flag, but do we also need to make changes
in the rest of this page to mention the new way for process
termination?

======
src/backend/postmaster/bgworker.c

TerminateBackgroundWorkersByOid:

2.
+/*
+ * Cancel background workers.
+ */
+void
+TerminateBackgroundWorkersByOid(Oid databaseId)

Let's also remove that word "Cancel" from the function comment and add
some more details.

SUGGESTION:
Terminate all background workers connected to the given database, if
they had requested it.

~~~

3.
+ /*
+ * Iterate through slots, looking for workers
+ * who connects to the given database.
+ */

"workers who connects" (??)

SUGGESTION
Iterate through slots, looking for workers connected to the given database.

======
src/backend/storage/ipc/procarray.c

4.
+ /*
+ * Terminate all background workers for this database, if
+ * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP)
+ */
+ TerminateBackgroundWorkersByOid(databaseId);
+

The comment is out-of-date now, because the flag name was changed to
BGWORKER_EXIT_AT_DATABASE_CHANGE.

======
src/include/postmaster/bgworker.h

5.
+/* Cancel background workers. */
+extern void TerminateBackgroundWorkersByOid(Oid databaseId);

There is already a commented extern for the
TerminateBackgroundWorker() function. IMO, just put this extern
alongside that one. You don't need a new comment.

~~~

6. The header comment in this file refers to the
TerminateBackgroundWorker() function. Probably, you need to check that
carefully to see if this new function should also be mentioned there.

======

FYI, I attached a v5 top-up diff for (some of) my above review
comments in case it helps.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Hi,

Thank you for your comments. I updated patch to v0006.

> -----Original Message-----
> From: Peter Smith mailto:smithpb2250@gmail.com
> 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 <li.evan.chao@gmail.com> 
> 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
youshould 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
betterdo 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_CHANGEwith 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
orenhance 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.

On 10/10/2025 11:04, Aya Iwata (Fujitsu) wrote:
> Thank you for your comments. I updated patch to v0006.My company also employs a large number of background workers. I
also
 
reviewed your patch.
It looks nice to be committed. The only question I have is whether to 
make sending a termination signal a default behaviour and let the flag 
deactivate it.

-- 
regards, Andrei Lepikhov,
pgEdge

Hi Iwata-San,

Some v6 comments.

======
doc/src/sgml/bgworker.sgml

1.
+      <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>


IMO, below is an improved wording for this:

<para>
 <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</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>

======
src/backend/postmaster/bgworker.c

+
+
+/*
+ * Terminate all background workers connected to the given database, if they
+ * had requested it.
+ */
+void
+TerminateBackgroundWorkersByOid(Oid databaseId)

Only 1 blank line is needed here.

======
src/include/postmaster/bgworker.h

+/*
+ * 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
+

That double-negative comment seems awkward. IMO, positive statements
are clearer. Also, do you think you should mention
BGWORKER_SHMEM_ACCESS, or was that deliberately omitted because
BGWORKER_BACKEND_DATABASE_CONNECTION requires that?

e.g. The suggested comment below is more closely aligned with the documentation.

SUGGESTION:
/*
 * Exit the bgworker when its database is dropped, renamed, moved to a
 * different tablespace, or used as a template for CREATE DATABASE.
 * Requires BGWORKER_SHMEM_ACCESS and BGWORKER_BACKEND_DATABASE_CONNECTION.
 */

======
src/test/modules/worker_spi/t/002_worker_terminate.pl

+sub launch_bgworker
+{
+ my ($node, $database, $testcase, $allow_terminate) = @_;
+ my $offset = -s $node->logfile;

Would '$request_terminate' be a more correct name for the $allow_terminate var?

======
src/test/modules/worker_spi/worker_spi.c

+ 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;
+

Would 'request_termination' be a more correct name for this new var?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Oct 13, 2025 at 9:28 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Iwata-San,
>
> Some v6 comments.
>
> ======
> doc/src/sgml/bgworker.sgml
>
> 1.
> +      <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>
>
>
> IMO, below is an improved wording for this:
>
> <para>
>  <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</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>
>
> ======
> src/backend/postmaster/bgworker.c
>
> +
> +
> +/*
> + * Terminate all background workers connected to the given database, if they
> + * had requested it.
> + */
> +void
> +TerminateBackgroundWorkersByOid(Oid databaseId)
>
> Only 1 blank line is needed here.
>
> ======
> src/include/postmaster/bgworker.h
>
> +/*
> + * 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
> +
>
> That double-negative comment seems awkward. IMO, positive statements
> are clearer. Also, do you think you should mention
> BGWORKER_SHMEM_ACCESS, or was that deliberately omitted because
> BGWORKER_BACKEND_DATABASE_CONNECTION requires that?
>
> e.g. The suggested comment below is more closely aligned with the documentation.
>
> SUGGESTION:
> /*
>  * Exit the bgworker when its database is dropped, renamed, moved to a
>  * different tablespace, or used as a template for CREATE DATABASE.
>  * Requires BGWORKER_SHMEM_ACCESS and BGWORKER_BACKEND_DATABASE_CONNECTION.
>  */
>
> ======
> src/test/modules/worker_spi/t/002_worker_terminate.pl
>
> +sub launch_bgworker
> +{
> + my ($node, $database, $testcase, $allow_terminate) = @_;
> + my $offset = -s $node->logfile;
>
> Would '$request_terminate' be a more correct name for the $allow_terminate var?
>
> ======
> src/test/modules/worker_spi/worker_spi.c
>
> + 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;
> +
>
> Would 'request_termination' be a more correct name for this new var?
>

There's another similar parameter name that I missed in the last post.
See /src/test/modules/worker_spi/worker_spi--1.0.sql

"  allow_termination boolean DEFAULT false)"

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

Dear Andrei,

> The only question I have is whether to
> make sending a termination signal a default behaviour and let the flag
> deactivate it.

I think it can be done in future enhancement.
We can change the default behavior based on the feedback from developers.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Peter san, 

Thank you for your comments. I updated this patch to v0007.

> -----Original Message-----
> From: Peter Smith <smithpb2250@gmail.com>
> Sent: Monday, October 13, 2025 10:20 AM
> To: Iwata, Aya/岩田 彩 <iwata.aya@fujitsu.com>
> Cc: Chao Li <li.evan.chao@gmail.com>; Kuroda, Hayato/黒田 隼人
> <kuroda.hayato@fujitsu.com>; Michael Paquier <michael@paquier.xyz>;
> pgsql-hackers <pgsql-hackers@postgresql.org>
> Subject: Re: [PROPOSAL] Termination of Background Workers for
> ALTER/DROP DATABASE
> 
> On Mon, Oct 13, 2025 at 9:28 AM Peter Smith <smithpb2250@gmail.com>
> wrote:
> >
> > Hi Iwata-San,
> >
> > Some v6 comments.
> >
> > ======
> > doc/src/sgml/bgworker.sgml
> >
> > 1.
> > +      <para>
> > +
> <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</primar
> y></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>
> >
> >
> > IMO, below is an improved wording for this:
> >
> > <para>
> >
> <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</primar
> y></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>

I updated this .sgml file. Thank you for your advice!

> > ======
> > src/backend/postmaster/bgworker.c
> >
> > +
> > +
> > +/*
> > + * Terminate all background workers connected to the given database, if
> they
> > + * had requested it.
> > + */
> > +void
> > +TerminateBackgroundWorkersByOid(Oid databaseId)
> >
> > Only 1 blank line is needed here.

I added 1 blank here.

> > ======
> > src/include/postmaster/bgworker.h
> >
> > +/*
> > + * 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
> > +
> >
> > That double-negative comment seems awkward. IMO, positive statements
> > are clearer. Also, do you think you should mention
> > BGWORKER_SHMEM_ACCESS, or was that deliberately omitted because
> > BGWORKER_BACKEND_DATABASE_CONNECTION requires that?
> >
> > e.g. The suggested comment below is more closely aligned with the
> documentation.
> >
> > SUGGESTION:
> > /*
> >  * Exit the bgworker when its database is dropped, renamed, moved to a
> >  * different tablespace, or used as a template for CREATE DATABASE.
> >  * Requires BGWORKER_SHMEM_ACCESS and
> BGWORKER_BACKEND_DATABASE_CONNECTION.
> >  */

I have replaced this code comment.

> > ======
> > src/test/modules/worker_spi/t/002_worker_terminate.pl
> >
> > +sub launch_bgworker
> > +{
> > + my ($node, $database, $testcase, $allow_terminate) = @_;
> > + my $offset = -s $node->logfile;
> >
> > Would '$request_terminate' be a more correct name for the $allow_terminate
> var?
> >
> > ======
> > src/test/modules/worker_spi/worker_spi.c
> >
> > + 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;
> > +
> >
> > Would 'request_termination' be a more correct name for this new var?
> >
> 
> There's another similar parameter name that I missed in the last post.
> See /src/test/modules/worker_spi/worker_spi--1.0.sql
> 
> "  allow_termination boolean DEFAULT false)"

I changed these parameter's name to "request_termination".


> On Mon, Oct 13, 2025 at 9:28 AM Peter Smith <smithpb2250@gmail.com>
> wrote:
...
> There's another similar parameter name that I missed in the last post.
> See /src/test/modules/worker_spi/worker_spi--1.0.sql
> 
> "  allow_termination boolean DEFAULT false)"

I also fixed this, too.

Best regards,
Aya Iwata
Fujitsu Limited

On Tue, Oct 14, 2025 at 04:36:01AM +0000, Hayato Kuroda (Fujitsu) wrote:
>> The only question I have is whether to
>> make sending a termination signal a default behaviour and let the flag
>> deactivate it.
>
> I think it can be done in future enhancement.
> We can change the default behavior based on the feedback from developers.

Yeah, I cannot agree with a change in the default behavior to cancel
the workers on a database touched by a database command.  This is a
behavior that exists since bgworkers are supported in tree in 9.3.  If
one is interested in making the workers more responsive, they could
just flip the flag switch.
--
Michael

On Wed, Oct 15, 2025 at 02:48:43AM +0000, Aya Iwata (Fujitsu) wrote:
> Thank you for your comments. I updated this patch to v0007.

+ * Exit the bgworker when its database is dropped, renamed, moved to a
+ * different tablespace, or used as a template for CREATE DATABASE.

I don't think that we need to list all these operations in details
here. We could just say "if its database is involved in a CREATE,
ALTER or DROP database command".  The docs should provide these
details, of course.

+#define BGWORKER_EXIT_AT_DATABASE_CHANGE       0x0004

Flag name works here.

# XXX This spends more than 5 seconds because the backend retries counting
# number of connecting processes 50 times. See CountOtherDBBackends().

And that's annoying.  Let's activate what I call the cheat mode for
this one: an injection point that, if defined, enforces a lower number
of tries when we loop over the workers to stop.  That would make the
test much faster when using a worker that should not be stopped,
without impacting the coverage.

I suspect that your new test 002_worker_terminate.pl has a race
condition in run_db_command(): are you sure that the bgworker has
enough time to be reported as stopped in the server logs once
safe_psql() finishes to run the database command given by the caller?
On very slow and/or loaded machines, particularly, that could hurt the
stability.  It seems to me that this should use a wait_for_log()
instead of a log_contains(), waiting for the worker to be reported as
stopped depending on the command executed.

Shouldn't this test also check that worker 0 (the one that does not
have the flag set) is still running at the end of the test?  I assume
that querying pg_stat_activity would be enough at the end of the
script.
--
Michael

Hi Chao-san, Michael san

Thank you for your comments! To accept your comment, I updated patch to v0008.


> From: Chao Li <li.evan.chao@gmail.com>
> Sent: Wednesday, October 15, 2025 12:37 PM
...
> By searching for “ByOid”, we can get some existing examples:
>
> ObjectAddress
> RefreshMatViewByOid(Oid matviewOid, bool is_create, bool skipData,
>                     bool concurrent, const char *queryString,
>                     QueryCompletion *qc)
>
> The function name clearly tells refresh MatView by Oid, so the oid in parameter is an old of mat view.
>
> ResultRelInfo *
> ExecLookupResultRelByOid(ModifyTableState *node, Oid resultoid,
>                          bool missing_ok, bool update_cache)
>
> The function name indicates ResultRel, so the oid is a result oid.
>
> AccessMethodInfo *
> findAccessMethodByOid(Oid oid)
>
> The function name tells to find access method, the the oid is an access method’s OID.
>
> You can find more …
>
> But in this patch, the function name only indeeds “terminate background workers”, while the oid is a database oid.
Maybewe can rename the  
> function to “TerminateDatabaseBgWorkersByOid()”.

Thank you. I changed the function name to "'TerminateBgWorkersByDbOid".
I prefer this name because there are not official terminology "Database background worker" and it's shorter.

> -----Original Message-----
> From: Michael Paquier <michael@paquier.xyz>
> Sent: Thursday, October 16, 2025 12:55 PM
...
> On Wed, Oct 15, 2025 at 02:48:43AM +0000, Aya Iwata (Fujitsu) wrote:

> + * Exit the bgworker when its database is dropped, renamed, moved to a
> + * different tablespace, or used as a template for CREATE DATABASE.
>
> I don't think that we need to list all these operations in details
> here. We could just say "if its database is involved in a CREATE,
> ALTER or DROP database command".  The docs should provide these
> details, of course.

Thank you. I fixed this .h file comment.

>
> +#define BGWORKER_EXIT_AT_DATABASE_CHANGE       0x0004
>
> Flag name works here.

Sorry, I cannot follow. Please tell me more details about this comment.

> # XXX This spends more than 5 seconds because the backend retries counting
> # number of connecting processes 50 times. See CountOtherDBBackends().
>
> And that's annoying.  Let's activate what I call the cheat mode for
> this one: an injection point that, if defined, enforces a lower number
> of tries when we loop over the workers to stop.  That would make the
> test much faster when using a worker that should not be stopped,
> without impacting the coverage.

I tried to implement your idea. Thanks Kuroda-san to help it.

> I suspect that your new test 002_worker_terminate.pl has a race
> condition in run_db_command(): are you sure that the bgworker has
> enough time to be reported as stopped in the server logs once
> safe_psql() finishes to run the database command given by the caller?
> On very slow and/or loaded machines, particularly, that could hurt the
> stability.  It seems to me that this should use a wait_for_log()
> instead of a log_contains(), waiting for the worker to be reported as
> stopped depending on the command executed.

I fixed this test to use wait_for_log() instead of log_contains().

> Shouldn't this test also check that worker 0 (the one that does not
> have the flag set) is still running at the end of the test?  I assume
> that querying pg_stat_activity would be enough at the end of the
> script.

Added. I cannot find a good way to clarify the worker is "worker 0" from the pg_stat_activity,
backend_type does not have the information. Thus I used the string "worker_spi dynamic" as the key.


Best regards,
Aya Iwata
Fujitsu Limited

Hi Iwata-San,

Some comments for the latest v8 patch.

======
src/backend/postmaster/bgworker.c

TerminateBgWorkersByBbOid:

1.
+void
+TerminateBgWorkersByDbOid(Oid oid)

Now the function name is more explicit, but that is not a good reason
to make the parameter name more vague.

IMO the parameter should still be "dbOid" or "databaseId" instead of
just "oid". (ditto for the extern in bgworker.h)

======
src/backend/storage/ipc/procarray.c


CountOtherDBBackends:

2.
+ /*
+ * Usually, we try 50 times with 100ms sleep between tries, making 5 sec
+ * total wait. If requested, it would be reduced to 10 times to shorten the
+ * test time.
+ */


The comment seemed vague to me. How about more like:

/*
 * Retry up to 50 times with 100ms between attempts (max 5s total).
 * Can be reduced to 10 attempts (max 1s total) to speed up tests.
 */

~~~

3.
+ for (tries = 0; tries < ntries; tries++)

'tries' can be declared as a for-loop variable.

~~~

4.
Something feels strange about this function name
(CountOtherDBBackends) which suggests it is just for counting stuff,
but in reality is more about exiting/terminating the workers. In fact
retuns a boolean, not a count. Compare this with this similarly named
"CountUserBackends" which really *is* doing what it says.

Can we give this function a better name, or is that out of scope for this patch?

======
src/test/modules/worker_spi/t/002_worker_terminate.pl

5.
+# Firstly register an injection point to make the test faster. Normally, it
+# spends more than 5 seconds because the backend retries, counting the number
+# of connecting processes 50 times, but now the counting would be done only 10
+# times. See CountOtherDBBackends().
+$node->safe_psql('postgres', "CREATE EXTENSION injection_points;");
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('reduce-ncounts', 'error');");
+

It seemed overkill to give details about what "normally" happens. I
think it is enough to have a simple comment here:

SUGGESTION
The injection point 'reduce-ncounts' reduces the number of backend
retries, allowing for shorter test runs. See CountOtherDBBackends().

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Oct 20, 2025 at 01:01:31PM +1100, Peter Smith wrote:
> Some comments for the latest v8 patch.

The comments of Peter apply to comments and parameters.  I am not
going down to these details in this message, these can be infinitely
tuned.

The injection point integration looks correct.  You are checking the
compile flag and if the extension is available in the installation
path, which should be enough.

+   if (IS_INJECTION_POINT_ATTACHED("reduce-ncounts"))
+       ntries = 10;

1s is much faster than the default of 5s, still I am wondering if this
cannot be brought down a bit more.  Dropping the worker still around
after the first test with CREATE DATABASE works here.

+# Confirm a background worker is still running
+$node->safe_psql(
+    "postgres", qq(
+        SELECT count(1) FROM pg_stat_activity
+        WHERE backend_type = 'worker_spi dynamic';));

This does not check that the worker that does not have the flag set is
still running: you are not feeding the output of this query to an is()
test.

+    is($result, 't', "dynamic bgworker launched");

In launch_bgworker(), this uses the same test description for all the
callers of this subroutine.  Let's prefix it with $testcase.

+void
+TerminateBgWorkersByDbOid(Oid oid)

FWIW, while reading this code, I was wondering about one improvement
that could show benefits for more extension code than only what we are
discussing here because external code has no access to
BackgroundWorkerSlot while holding the LWLock BackgroundWorkerLock in
a single loop, by rewriting this new routine with something like that:
void TerminateBackgroundWorkerMatchin(
bool (*do_terminate) (int pid, BackgroundWorker *, Datum))

Then the per-database termination would be a custom routine, defined
also in bgworker.c.  Other extension code could define their own
filtering callback routine.  Just an idea in passing, to let extension
code take more actions on bgworker slots in use-based on a PGPROC
entry, like a role ID for example, or it could be a different factor.
Feel free to dislike such a funky idea if you do not like it and say
so, of course.
--
Michael

Hi Peter-san, Michael-san,

Thank you for your comments.
I updated patch to v0009. Please review attached patch.

> -----Original Message-----
> From: Peter Smith <smithpb2250@gmail.com>
> Sent: Monday, October 20, 2025 11:02 AM

> Some comments for the latest v8 patch.
> 
> ======
> src/backend/postmaster/bgworker.c
> 
> TerminateBgWorkersByBbOid:
> 
> 1.
> +void
> +TerminateBgWorkersByDbOid(Oid oid)
> 
> Now the function name is more explicit, but that is not a good reason
> to make the parameter name more vague.
> 
> IMO the parameter should still be "dbOid" or "databaseId" instead of
> just "oid". (ditto for the extern in bgworker.h)

I agree with you. I reverted parameter name to databaseId.


> ======
> src/backend/storage/ipc/procarray.c
> 
> 
> CountOtherDBBackends:
> 
> 2.
> + /*
> + * Usually, we try 50 times with 100ms sleep between tries, making 5 sec
> + * total wait. If requested, it would be reduced to 10 times to shorten the
> + * test time.
> + */
> 
> 
> The comment seemed vague to me. How about more like:
> 
> /*
>  * Retry up to 50 times with 100ms between attempts (max 5s total).
>  * Can be reduced to 10 attempts (max 1s total) to speed up tests.
>  */

Thank you. I updated this comment.

> 3.
> + for (tries = 0; tries < ntries; tries++)
> 
> 'tries' can be declared as a for-loop variable.

I have declared "int" within the for-loop.

> ~~~
> 
> 4.
> Something feels strange about this function name
> (CountOtherDBBackends) which suggests it is just for counting stuff,
> but in reality is more about exiting/terminating the workers. In fact
> retuns a boolean, not a count. Compare this with this similarly named
> "CountUserBackends" which really *is* doing what it says.
> 
> Can we give this function a better name, or is that out of scope for this patch?

I think this is out of scope because existing code have terminated autovacuum process by SIGTERM.
It can be discussed separately.
I just added a comment to this function "background workers would also be terminated".

> ======
> src/test/modules/worker_spi/t/002_worker_terminate.pl
> 
> 5.
> +# Firstly register an injection point to make the test faster. Normally, it
> +# spends more than 5 seconds because the backend retries, counting the
> number
> +# of connecting processes 50 times, but now the counting would be done only
> 10
> +# times. See CountOtherDBBackends().
> +$node->safe_psql('postgres', "CREATE EXTENSION injection_points;");
> +$node->safe_psql('postgres',
> + "SELECT injection_points_attach('reduce-ncounts', 'error');");
> +
> 
> It seemed overkill to give details about what "normally" happens. I
> think it is enough to have a simple comment here:
> 
> SUGGESTION
> The injection point 'reduce-ncounts' reduces the number of backend
> retries, allowing for shorter test runs. See CountOtherDBBackends().

Thank you for your suggestion. I updated this comment.

> -----Original Message-----
> From: Michael Paquier <michael@paquier.xyz>
> Sent: Monday, October 20, 2025 1:33 PM

> On Mon, Oct 20, 2025 at 01:01:31PM +1100, Peter Smith wrote:
> > Some comments for the latest v8 patch.
> 
> The comments of Peter apply to comments and parameters.  I am not
> going down to these details in this message, these can be infinitely
> tuned.
> 
> The injection point integration looks correct.  You are checking the
> compile flag and if the extension is available in the installation
> path, which should be enough.
> 
> +   if (IS_INJECTION_POINT_ATTACHED("reduce-ncounts"))
> +       ntries = 10;
> 
> 1s is much faster than the default of 5s, still I am wondering if this
> cannot be brought down a bit more.  Dropping the worker still around
> after the first test with CREATE DATABASE works here.

Thank you. I updated ntries to 3.

> +# Confirm a background worker is still running
> +$node->safe_psql(
> +    "postgres", qq(
> +        SELECT count(1) FROM pg_stat_activity
> +        WHERE backend_type = 'worker_spi dynamic';));
> 
> This does not check that the worker that does not have the flag set is
> still running: you are not feeding the output of this query to an is()
> test.
> 
> +    is($result, 't', "dynamic bgworker launched");
> 
> In launch_bgworker(), this uses the same test description for all the
> callers of this subroutine.  Let's prefix it with $testcase.

I added $testcase. Is it same as your expectations?

> +void
> +TerminateBgWorkersByDbOid(Oid oid)
> 
> FWIW, while reading this code, I was wondering about one improvement
> that could show benefits for more extension code than only what we are
> discussing here because external code has no access to
> BackgroundWorkerSlot while holding the LWLock BackgroundWorkerLock in
> a single loop, by rewriting this new routine with something like that:
> void TerminateBackgroundWorkerMatchin(
> bool (*do_terminate) (int pid, BackgroundWorker *, Datum))
> 
> Then the per-database termination would be a custom routine, defined
> also in bgworker.c.  Other extension code could define their own
> filtering callback routine.  Just an idea in passing, to let extension
> code take more actions on bgworker slots in use-based on a PGPROC
> entry, like a role ID for example, or it could be a different factor.
> Feel free to dislike such a funky idea if you do not like it and say
> so, of course.

Thank you for your advice.
I'd like to address that, but I couldn't figure out how to do it on my own. 
Could you please describe it more?

Regards,
Aya Iwata
Fujitsu Limited

Hi

Perhaps you haven't seen my previous post.

> > FWIW, while reading this code, I was wondering about one improvement
> > that could show benefits for more extension code than only what we are
> > discussing here because external code has no access to
> > BackgroundWorkerSlot while holding the LWLock BackgroundWorkerLock in
> > a single loop, by rewriting this new routine with something like that:
> > void TerminateBackgroundWorkerMatchin(
> > bool (*do_terminate) (int pid, BackgroundWorker *, Datum))
> >
> > Then the per-database termination would be a custom routine, defined
> > also in bgworker.c.  Other extension code could define their own
> > filtering callback routine.  Just an idea in passing, to let extension
> > code take more actions on bgworker slots in use-based on a PGPROC
> > entry, like a role ID for example, or it could be a different factor.
> > Feel free to dislike such a funky idea if you do not like it and say
> > so, of course.
> 
> Thank you for your advice.
> I'd like to address that, but I couldn't figure out how to do it on my own.
> Could you please describe it more?

I'd like to adapt the patch for this, so could you tell me with the details?

Regards,
Aya Iwata
Fujitsu Limited

Hi

Thank you for your review.

> From: Pavel Stehule <pavel.stehule@gmail.com> 
> Sent: Sunday, December 14, 2025 4:40 PM
> To: Michael Paquier <michael@paquier.xyz>
> Cc: Iwata, Aya/岩田 彩 <iwata.aya@fujitsu.com>; Peter Smith <smithpb2250@gmail.com>; Chao Li <li.evan.chao@gmail.com>;
Kuroda,Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com>; pgsql-hackers <pgsql-hackers@postgresql.org>
 
> Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
> 
> +#define BGWORKER_EXIT_AT_DATABASE_CHANGE       0x0004 
> 
> I am checking this patch, and I think so used names can be little bit confusing
> 
> BGWORKER_EXIT_AT_DATABASE_CHANGE - it is used for disconnecting workers on the template database, and this database
isnot changing.
 
> 
> TerminateBgWorkersByDbOid - it doesn't terminate all workers, but only workers with some special flags
> 
> Maybe BGWORKER_INTERRUPTABLE and TerminateInterruptableBgWorkersByDbOid ?

Thank you for your advice.
I changed the name of a function and a flag. 

> Another question is if this cancellation should be implicit and should not require some special flag.
> 
> When I want to disconnect connections to database when I do drop, I have to use FORCE flag
> 
> So maybe there should be ALTER DATABASE ... RENAME ... FORCE - or if FORCE can terminare all workers (without special
FLAG)?
 

For the proposed feature, we've added a flag allowing each extension developer to decide whether to terminate it via
DROP/ALTERDATABASE.
 
Adding a FORCE option to ALTER to let database definition modifiers decide whether to force termination of background
workersmight be better discussed in a separate thread.
 

Best Regards,
Aya Iwata

Hi Pavel-san,

>> So maybe there should be ALTER DATABASE ... RENAME ... FORCE - or if FORCE can terminare all workers (without
specialFLAG) ?
 
> 
> For the proposed feature, we've added a flag allowing each extension developer to decide whether to terminate it via
DROP/ALTERDATABASE.
 
> Adding a FORCE option to ALTER to let database definition modifiers decide whether to force termination of background
workersmight be better discussed in a separate thread.
 
> 
> When I thought about it - there can be a second alternative.
> 
> Introduce a pair of flags BGWORKER_INTERRUPTABLE and BGWORKER_PROTECTED (the names can be enhanced or changed).
BGWORKER_INTERRUPTABLEcan be default. 
 
> ALTER DATABASE RENAME and related commands can stop any non protected workers. ALTER DATABASE RENAME FORCE can stop
anyworkers (including protected). 
 

I can't image any use cases for BGWORKER_PROTECTED. Do you have any idea?
Also, I think the parameter settings might get a complicated.
If we start discussing the "FORCE" option, it is better to think about this parameter.

> Is there any reason why BGWORKER_INTERRUPTABLE cannot be default? Probably nobody would block some possibly common
operationson database level without strong reason.
 

As Michael-san mentioned in a previous email, this behavior has remained unchanged since bgworkers were introduced in
v9.3.
 
I don't see a compelling reason to alter it now.  Additionally, this specification can be modified later.

Best Regards,
Aya Iwata

Hi Pavel-san

Thank you for your feedback!

> From: Pavel Stehule <pavel.stehule@gmail.com> 
> Sent: Thursday, December 18, 2025 12:33 AM
> To: Iwata, Aya/岩田 彩 <iwata.aya@fujitsu.com>
> Cc: Michael Paquier <michael@paquier.xyz>; Peter Smith <smithpb2250@gmail.com>; Chao Li <li.evan.chao@gmail.com>;
Kuroda,Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com>; pgsql-hackers <pgsql-hackers@postgresql.org>
 
> Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
>> Hi Pavel-san,
>> 
>> >> So maybe there should be ALTER DATABASE ... RENAME ... FORCE - or if FORCE can terminare all workers (without
specialFLAG) ?
 
>> > 
>> > For the proposed feature, we've added a flag allowing each extension developer to decide whether to terminate it
viaDROP/ALTER DATABASE.
 
>> > Adding a FORCE option to ALTER to let database definition modifiers decide whether to force termination of
backgroundworkers might be better discussed in a separate thread.
 
>> > 
>> > When I thought about it - there can be a second alternative.
>> > 
>> > Introduce a pair of flags BGWORKER_INTERRUPTABLE and BGWORKER_PROTECTED (the names can be enhanced or changed).
BGWORKER_INTERRUPTABLEcan be default. 
 
>> > ALTER DATABASE RENAME and related commands can stop any non protected workers. ALTER DATABASE RENAME FORCE can
stopany workers (including protected). 
 
>> 
>> I can't image any use cases for BGWORKER_PROTECTED. Do you have any idea?
>> Also, I think the parameter settings might get a complicated.
>> If we start discussing the "FORCE" option, it is better to think about this parameter.
>> 
>> > Is there any reason why BGWORKER_INTERRUPTABLE cannot be default? Probably nobody would block some possibly common
operationson database level without strong reason.
 
>> 
>> As Michael-san mentioned in a previous email, this behavior has remained unchanged since bgworkers were introduced
inv9.3. 
 
>> I don't see a compelling reason to alter it now. Additionally, this specification can be modified later.

> I understand the request for unchanging behaviour - but I am not sure if this concept is really helpful - or if the
namingis best. I am afraid so this feature without changing the workers code is useless (and maybe it is wanted).
 

It is our intention; this feature would enable when developer expressly set.

> Any worker should be interruptable by sigterm. And then the name BGWORKER_INTERRUPTABLE is little bit vague. Maybe
somelike BGWORKER_CAREFREE_INTERRUPTABLE can be better (or some like this - maybe BGWORKER_CANCELABLE)? This can be a
signalfrom bgworker's authors - it is ok to kill the worker anytime when it is necessary. 
 

That's right, "interruptable" may not be appropriate. This is because even bgworkers without this flag set can be
interruptedby sigterm.
 
Hmm, I feel these ideas may not be clear what it does. Do someone have other idea?

> Some workers can have the flag BGW_NEVER_RESTART - cannot be used as signal so this worker is protected, and others
canbe terminated safely, because they will be restarted after 60 seconds?
 

In my understanding, you are suggesting that the bgworker which set bgw_restart_time as BGW_NEVER_RESTART is not
terminatedby DROP/ALTER, right? 
 
Do you think what kind of use cases might there be?

Best Regards,
Aya Iwata

Hi

> FWIW, while reading this code, I was wondering about one improvement
> that could show benefits for more extension code than only what we are
> discussing here because external code has no access to
> BackgroundWorkerSlot while holding the LWLock BackgroundWorkerLock in
> a single loop, by rewriting this new routine with something like that:
> void TerminateBackgroundWorkerMatchin(
> bool (*do_terminate) (int pid, BackgroundWorker *, Datum))
>
> Then the per-database termination would be a custom routine, defined
> also in bgworker.c.  Other extension code could define their own
> filtering callback routine.  Just an idea in passing, to let extension
> code take more actions on bgworker slots in use-based on a PGPROC
> entry, like a role ID for example, or it could be a different factor.
> Feel free to dislike such a funky idea if you do not like it and say
> so, of course.

I'm sorry for the delayed response.
I tried implementing a callback function within the for loop to allow setting the termination
condition myself. However, I felt this method had few use-cases. Since LWlock exists in here, it can
only handle lightweight operations.
Therefore, I'd prefer not to include it at this time. If I later think of a useful scenario for it, I'll
reconsider about this.

Best Regards,
Aya Iwata

Hi

https://www.postgresql.org/message-id/aPBoXXW3XuwiIsHG%40paquier.xyz

Based on previous discussions, we had configured DROP/ALTER DATABASE to
terminate only background workers with specific parameters set.

To reconsider the default behavior, I created the following patches:
0001 is the patch for the existing implementation.
0002 is the patch modified based on Pavel-san's suggestion.

The 0002 patch changes the default behavior to terminate background workers
when DROP/ALTER DATABASE is executed. It also includes a test set
for this change.
For background workers that should not be terminated, setting
the BGWORKER_PROTECTED parameter prevents this.

The relationship with the FORCE option during DROP DATABASE is as follows:

FORCE option present, flag present... TERMINATE
FORCE option present, flag absent... TERMINATE
FORCE option absent, flag present... Does not TERMINATE due to the flag
FORCE option absent, flag absent... TERMINATE

I would like to know your comments on which implementation is preferable.

Regards,
Aya Iwata

On Fri, Dec 26, 2025 at 12:04:47PM +0100, Pavel Stehule wrote:
> I am not sure if I understand what you prefer.

Seems to me that Iwata-san sides with compatibility.  Spoiler alert: I
do side with compatibility.  See below for more details.

> 1. When bgworker is marked as INTERRUPTIBLE, then can be cancelled
> immediately - there is full agreement - I don't see any problem - just
> probably almost all bgworkers will be not be marked as INTERRUPTIBLE

Yes, these can and should be stopped.

> 2.  When bgworker is not marked as INTERRUPTIBLE, then can it be cancelled?
> How dangerous is this? If it cannot be cancelled, then ALTER should wait on
> lock forever, or there should be some special timeout - like CREATE, DROP
> DATABASE and waiting on template databases.

Cancellation is a different thing.  bgworkers can be tweaked to answer
to specific signals with their own handlers.  What we are discussing
here is if a signal should be sent to a bgworker or not when
performing specific operations.

> 3. If the user needs to execute ALTER, then probably he manually terminates
> the worker any time. Are currently used workers safe against terminating?
> We know so mostly workers will be restarted after timeout - and then it is
> "safe".  It is really safe? Can we expect it? If it is safe, then we can
> implement FORCE clause. Probably any worker can be restarted - and without
> safety it is hard to imagine using in the production.

That would be up to an extension developer.  The new behavior can be
useful in some cases.  Forcing it by default could lead to unwanted
results.

> I think there is a fundamental question that should be solved before - what
> is a risk of canceling bgworker? The possible reply is so there is not any
> risk for correctly implemented bgworkers. And then there is a question if
> we still need the flag INTERRUPTIBLE? My opinion for this case is probably
> yes, because cancelling can introduce some bigger latency.

I can think about two reasons at the top of my mind, at least:
1) Making the background workers non-interruptible is the default
behavior that we have since 9.3.  Changing the default by allowin
these to be interrupted could lead to a silent behavior change that
extension developers are not aware of because the bgworker lubrary
code would still be compiled as-is, and would still be able to start
correctly.  This links to my second point.
2) Operator error, and these tend to happen a lot.  Somebody with the
right to create a database could decide to use as template a database
that a bgworker is linked to, leading to failures with operations
background workers expect to be able to achieve while online if we
change the default, because it does things that are critical and
should not be interrupted (one could compare that to what an
autovacuum worker does with an antiwraparound work, perhaps, which
cannot be interrupted).

As a whole, I think that we have more advantages in keeping the
default, making the possibility to make a bgworker interruptible an
opt-in choice is extra cream that can be added on top of a cake, and
one is free to add the extra cream to their portion of the cake.
--
Michael

On Fri, Dec 26, 2025 at 10:17:27AM +0000, Ryo Matsumura (Fujitsu) wrote:
> +1 to Allow-background-workers-to-be-terminated
>
> The result is same, so I think it's better to prioritize compatibility.
>
> PGWORKER_PROTECTED would be used in scenarios like the following:
> Existing features are probably not designed to be forcibly stopped.
> Therefore, all existing features should have PROTECTED applied to them.
> Most newly implemented features will also have PROTECTED applied because it requires less thought and is safer.
> Only considerate developers of features that can easily guarantee safety would adopt the default.

We could design things so as we have a second flag to force a bgworker
to be non-interuptible, then we could force that either the
interruptible flag or the non-interruptible flag should be set.  What
is mentioned as a problem is that 0 implies that the non-interruptible
is enforced.  I don't think that we would have much to gain by doing
that, as it would just lead to extension breakages that we can avoid.

> In conclusion, this is no different from BGWORKER_INTERRUPTABLE.
> Therefore, I think it's better to prioritize compatibility.

Looking finally at the patch, I like the simplicity of what you are
doing here.

+      Requires both <literal>BGWORKER_SHMEM_ACCESS</literal> and
+      <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal>.

SanityCheckBackgroundWorker() does not enforce this check when
registering a bgworker.  But shouldn't we do so when the interruptible
flag is set?

+void
+TerminateInterruptableBgWorkersByDbOid(Oid databaseId)

Fine by me to aim for simplicity with this interface, discarding my
previous fancy comment about the extensibility we could do here.
Matsumura-san and I have also discussed a bit that offline at the last
JPUG, where I said that I'm OK with simpler at the end.

One issue with the test as written, as of run_db_command(), is that we
make sure that a worker is stopped by scanning the output of the logs.
This approach may detect incorrect patterns, unfortunately.  For
example, if the termination logic has a bug it may be possible that
the worker found as terminated is the first one created by the test,
which we expect to always run.  While the log is mandatory to have, I
have a suggestion to make that even better: let's keep track in
run_db_command() of the PIDs of the worker processes we expect to
exist after running each database command, then make sure that the
list of PIDs match with what we expect.  This is a bit simpler in the
case of this test as we only expect one matching PID.
--
Michael

Hi Iwata-San.

It is not at all clear to me if you are advocating for the flag to be
called INTERRUPTABLE or to be called PROTECTED?

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.

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?

~~~

Anyway, just in case you are PROTECTED is still under consideration,
then below are some review comments for the v11-0002 patch.

======
doc/src/sgml/bgworker.sgml

1a.
The commit message says that "default behavior to Terminate." which I
assumed means that by default, the PROTECTED is *note* set. But the
default behaviour is not mentioned in the SGML help?

1b.
Also, the INTERRUPTABLE flag was documented that it required both
BGWORKER_SHMEM_ACCESS and BGWORKER_BACKEND_DATABASE_CONNECTION as
well. But did it make sense to document that those are both still
required for PROTECTED?

======
src/backend/postmaster/bgworker.c

2.
TerminateInterruptableBgWorkersByDbOid:

AFAIK, you only called the function this name because of the
INTERRUPTABLE flag. But, if you are going to use PROTECTED instead,
then the function name should likewise change to something like
TerminateUnprotectedBgWorkersByDbOid.

======
src/backend/storage/ipc/procarray.c

CountOtherDBBackends:

3.
  /*
  * Terminate all background workers for this database, if they had
- * requested it (BGWORKER_INTERRUPTABLE)
+ * requested it (BGWORKER_PROTECTED)
  */

The comment seems wrong because BGWORKER_PROTECTED means do NOT
terminate it because it is protected. So the comment should be more
like:
Terminate all background workers for this database, unless they are
flagged as BGWORKER_PROTECTED.

======
src/include/postmaster/bgworker.h

4.
-#define BGWORKER_INTERRUPTABLE 0x0004
+#define BGWORKER_PROTECTED 0x0004

You cannot change the name to have the opposite meaning, without also
rewriting the preceding code comment.

======
src/test/modules/worker_spi/t/002_worker_terminate.pl

5.
-# Ensure CREATE DATABASE WITH TEMPLATE fails because background worker retains
+# Ensure CREATE DATABASE WITH TEMPLATE sucseeds because background
worker retains

Typo - "sucseeds"
Typo - "background worker retains" ?? Do you mean "remains" ??

~~~

6.
The code below:
    my $result = $node->safe_psql(
        $database, qq(
        SELECT worker_spi_launch($testcase, oid, 0, '{}',
$request_terminate) IS NOT NULL
        FROM pg_database WHERE datname = '$database';
    ));
...

seems contradictary because the 4th parameter ($request_terminate) of
worker_spi_launch() was changed to *prevent* termination, not request
it.

e.g.
bool prevent_termination = PG_GETARG_BOOL(4);

It makes me doubtful about the testcode validity/correctness, given
that the parameter now has the opposite meaning from patch 0001, but
still the test is passing.

======
[1] https://www.postgresql.org/message-id/aVHQt-asgKx5dnap%40paquier.xyz

Kind Regards,
Peter Smith.
Fujitsu Australia

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

On Mon, Jan 5, 2026 at 2:59 PM Michael Paquier <michael@paquier.xyz> wrote:
>
...
> 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?
> --

Here are my (mostly trivial) review comments for the latest patch v12-0001.

Otherwise, LGTM.

======
src/backend/postmaster/bgworker.c

1. nit
+ * Terminate all background workers connected to the given database, if they
+ * had requested it.

In procarray.c you changed the equivalent comment to "if they *have*
requested it."

~~~

2. nit
+ for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)

The ++slotno is inconsistent with slotno++ of the preceding function loop.

It is a matter of taste... YMMV.

======
src/backend/storage/ipc/procarray.c

3
- * 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.

Is that code comment change strictly correct?

IIUC, the background workers are stopped by
TerminateInterruptibleBgWorkersByDbOid using
PMSIGNAL_BACKGROUND_WORKER_CHANGE, not SIGTERM.

======
.../worker_spi/t/002_worker_terminate.pl

4. nit
+sub launch_bgworker
+{
+ my ($node, $database, $testcase, $request_terminate) = @_;

Everywhere else, you called this flag 'interruptible'; so, should you
also change $request_terminate to $interruptible?

~~~

5. nit
+sub run_db_command

Would a better name for this be more like 'run_bgworker_interruptible_test'

~~~

6.
+# 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';));

Indentation of the "SELECT"?

======
doc/src/sgml/bgworker.sgml

7.
+      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>.

This looks OK in the SGML, but not when it is rendered. The multiple
commands are separated only by commas, so because the font changes are
subtle, it ends up looking like one big comma-separated command.

How about using an itemised list like:

+      <itemizedlist>
+       <listitem><para><command>DROP DATABASE</command></para></listitem>
+       <listitem><para><command>ALTER DATABASE RENAME
TO</command></para></listitem>
+       <listitem><para><command>ALTER DATABASE SET
TABLESPACE</command></para></listitem>
+       <listitem><para><command>CREATE DATABASE</command></para></listitem>
+      </itemizedlist>

This renders nicely.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Hi 

Thank you for your comments and suggestions on the previous version patches!    
I believe this feature become much better.

I've created a v13 patch incorporating Peter-san's suggestions.

> One issue with the test as written, as of run_db_command(), is that we
> make sure that a worker is stopped by scanning the output of the logs.
> This approach may detect incorrect patterns, unfortunately.  For
> example, if the termination logic has a bug it may be possible that
> the worker found as terminated is the first one created by the test,
> which we expect to always run.  While the log is mandatory to have, I
> have a suggestion to make that even better: let's keep track in
> run_db_command() of the PIDs of the worker processes we expect to
> exist after running each database command, then make sure that the
> list of PIDs match with what we expect.  This is a bit simpler in the
> case of this test as we only expect one matching PID.

I've also reviewed the test set code comparing PIDs. I think this is acceptable.

Best Regards,
Aya Iwata

On Tue, Jan 6, 2026 at 2:06 AM Aya Iwata (Fujitsu)
<iwata.aya@fujitsu.com> wrote:
>
> Hi
>
> Thank you for your comments and suggestions on the previous version patches!
> I believe this feature become much better.
>
> I've created a v13 patch incorporating Peter-san's suggestions.
>

Some review comments for v13-0001.

======
src/backend/storage/ipc/procarray.c

CountOtherDBBackends:

1.
  * 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 PMSIGNAL_BACKGROUND_WORKER_CHANGE, but normal
+ * user backends are just waited for.

This did not seem like the correct fix for my previous review comment
[1, comment #3], because the autovacuum backends are still killed with
SIGTERM, right?

======
.../worker_spi/t/002_worker_terminate.pl

2.
+# 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';));

The indentation of the "SELECT" still does not look correct to me. Did
you run pgperltidy on this file?

======
[1] https://www.postgresql.org/message-id/CAHut%2BPtSVYKU4vfaRev4FMdbeZ3ukvxRy4X7uK05jv_9WMYafA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Jan 06, 2026 at 08:18:39AM +1100, Peter Smith wrote:
> This did not seem like the correct fix for my previous review comment
> [1, comment #3], because the autovacuum backends are still killed with
> SIGTERM, right?

Yes, you are right that this creates a confusing mix.  Bgworkers have
no relationship with SIGTERM in this code.  I have added a sentence to
document the new behavior for interruptible bgworkers, after the
existing ones.  One thing that was also missing is the fact that the
wait can be shortened with the injection point.

> 2.
> +# 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';));
>
> The indentation of the "SELECT" still does not look correct to me. Did
> you run pgperltidy on this file?

perltidy is happy with that, because it considers the string within
the quoted qq() area as something fine.  At the end this is just a tab
vs whitespace issue, you are right that this should use whitespaces.

I have done a couple of test runs in the CI to be sure, and noticed no
instability in the tests, so applied.  I'll keep an eye on the CFbot
and the buildfarm for the next few days.
--
Michael