Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE - Mailing list pgsql-hackers

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