RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE

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