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

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