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(at)gmail(dot)com>
> Sent: Monday, October 13, 2025 10:20 AM
> To: Iwata, Aya/岩田 彩 <iwata(dot)aya(at)fujitsu(dot)com>
> Cc: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>; Kuroda, Hayato/黒田 隼人
> <kuroda(dot)hayato(at)fujitsu(dot)com>; Michael Paquier <michael(at)paquier(dot)xyz>;
> pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
> Subject: Re: [PROPOSAL] Termination of Background Workers for
> ALTER/DROP DATABASE
>
> On Mon, Oct 13, 2025 at 9:28 AM Peter Smith <smithpb2250(at)gmail(dot)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(at)gmail(dot)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