Thread

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

Aya Iwata (Fujitsu) <iwata.aya@fujitsu.com> — 2025-10-08T12:26:05Z

Hi Kuroda-san, Peter-san,

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

> -----Original Message-----
> From: Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.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@gmail.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