MIME-Version: 1.0
References: 
 <OS7PR01MB11964335F36BE41021B62EAE8EAE4A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <OSCPR01MB149662AEA64F4E66F494C2584F5E3A@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <aOM02tVFtXmuR0M1@paquier.xyz>
 <OS7PR01MB119648846BA926C096CE016B6EAE3A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <CAHut+Pt4Tn1bQYCsYeUt_gtcSB-KOTtRB70SLghkpsjfKGsm7w@mail.gmail.com>
 <OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Wed, 8 Oct 2025 08:39:43 +1100
Message-ID: 
 <CAHut+Pu-nxNice547eGEW2O3hdRcFbPYWF4HiqktZO19X3Ah-g@mail.gmail.com>
Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP
 DATABASE
To: "Aya Iwata (Fujitsu)" <iwata.aya@fujitsu.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPu-nxNice547eGEW2O3hdRcFbPYWF4HiqktZO19X3Ah-g%40mail.gmail.com>
Precedence: bulk

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