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

Hi Iwata-San,

Some v6 comments.

======
doc/src/sgml/bgworker.sgml

1.
+      <para>
+       <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</primary></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</primary></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>

======
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.

======
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.
 */

======
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?

======
Kind Regards,
Peter Smith.
Fujitsu Australia