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

Hi Kuroda-san, Peter-san,

Thank you for your review comments! I updated patch to v0004.

> -----Original Message-----
> From: Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)com>
> Sent: Tuesday, October 7, 2025 8:49 PM

> ```
> + /* 2nd, compare databaseId. */
> + if (proc && proc->databaseId == databaseId)
> ```
>
> Here should describes what are you trying to do. How about something like:
> Checks the connecting database of the worker, and instruct the postmaster to
> terminate it if needed

This comment has been removed. We can know code's intent without comments.

> ```
> + /*
> + * Cancel background workers by admin commands.
> + */
> + CancelBackgroundWorkers(databaseId);
> ```
>
> Since we removed the flag, the comment is outdated.

Thank you. I changed this comment:
"if set the bgw_flags, cancel background workers."

> ```
> -
> typedef void (*bgworker_main_type) (Datum main_arg);
> ```
>
> This change is not related with this patch.

I removed this change.

> ```
> @@ -361,7 +361,8 @@ _PG_init(void)
> /* set up common data for all our workers */
> memset(&worker, 0, sizeof(worker));
> worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
> - BGWORKER_BACKEND_DATABASE_CONNECTION;
> + BGWORKER_BACKEND_DATABASE_CONNECTION |
> + BGWORKER_EXIT_AT_DATABASE_DROP;
> ```
>
> The new flag was added to both static and dynamic background workers. So,
> how about
> testing both? I think it is enough to use one of case, like ALTER DATABASE SET
> TABLESPACE.

I removed this BGWORKER_EXIT_AT_DATABASE_DROP from the patch.

> -----Original Message-----
> From: Peter Smith <smithpb2250(at)gmail(dot)com>

> ======
> doc/src/sgml/bgworker.sgml
>
> 1.

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

Thank you. I checked and replaced the doc.

> ======
>
>
> 2.

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

I removed this comment.

> 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

I removed this comment, too.

> 2c.
> + /* 2nd, compare databaseId. */
> + if (proc && proc->databaseId == databaseId)
> Remove that one. It is the same as the code.

I removed this comment, too.

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

I put this comment to before the for 'loop'.
And I changed comment like this:

/*
* Set terminate flag in shared memory, unless slot has
* been used.
*/

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

Thank you. I changed the source code comments based on this comment.

Regards,
Aya Iwata
Fujitsu Limited