Обсуждение: [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