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

On Mon, Jan 5, 2026 at 2:59 PM Michael Paquier <michael@paquier.xyz> wrote:
>
...
> The timing is interesting, I have put my hands on this patch this
> morning before you sent your last email, and adjusted the thing in
> many ways, finishing with the attached.  This includes changes in the
> tests to address what I found was lacking and slightly incorrect, new
> names for the flag and its related variables, copyright update to
> 2026, as well as an additional sanity check when starting the workers,
> leading to the updated version attached.  The CI is happy with it.
>
> Thoughts or comments about that?
> --

Here are my (mostly trivial) review comments for the latest patch v12-0001.

Otherwise, LGTM.

======
src/backend/postmaster/bgworker.c

1. nit
+ * Terminate all background workers connected to the given database, if they
+ * had requested it.

In procarray.c you changed the equivalent comment to "if they *have*
requested it."

~~~

2. nit
+ for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)

The ++slotno is inconsistent with slotno++ of the preceding function loop.

It is a matter of taste... YMMV.

======
src/backend/storage/ipc/procarray.c

3
- * for them to exit.  Autovacuum backends are encouraged to exit early by
- * sending them SIGTERM, but normal user backends are just waited for.
+ * for them to exit.  Autovacuum backends and background workers are encouraged
+ * to exit early by sending them SIGTERM, but normal user backends are just
+ * waited for.

Is that code comment change strictly correct?

IIUC, the background workers are stopped by
TerminateInterruptibleBgWorkersByDbOid using
PMSIGNAL_BACKGROUND_WORKER_CHANGE, not SIGTERM.

======
.../worker_spi/t/002_worker_terminate.pl

4. nit
+sub launch_bgworker
+{
+ my ($node, $database, $testcase, $request_terminate) = @_;

Everywhere else, you called this flag 'interruptible'; so, should you
also change $request_terminate to $interruptible?

~~~

5. nit
+sub run_db_command

Would a better name for this be more like 'run_bgworker_interruptible_test'

~~~

6.
+# Confirm that the non-interruptible bgworker is still running.
+my $result = $node->safe_psql(
+ "postgres", qq(
+        SELECT count(1) FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi dynamic';));

Indentation of the "SELECT"?

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

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

This looks OK in the SGML, but not when it is rendered. The multiple
commands are separated only by commas, so because the font changes are
subtle, it ends up looking like one big comma-separated command.

How about using an itemised list like:

+      <itemizedlist>
+       <listitem><para><command>DROP DATABASE</command></para></listitem>
+       <listitem><para><command>ALTER DATABASE RENAME
TO</command></para></listitem>
+       <listitem><para><command>ALTER DATABASE SET
TABLESPACE</command></para></listitem>
+       <listitem><para><command>CREATE DATABASE</command></para></listitem>
+      </itemizedlist>

This renders nicely.

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