public inbox for [email protected]  

help / color / mirror / Atom feed

From: Peter Smith <[email protected]>
To: Aya Iwata (Fujitsu) <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
Date: Wed, 8 Oct 2025 08:39:43 +1100
Message-ID: <CAHut+Pu-nxNice547eGEW2O3hdRcFbPYWF4HiqktZO19X3Ah-g@mail.gmail.com> (raw)
In-Reply-To: <OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
References: <OS7PR01MB11964335F36BE41021B62EAE8EAE4A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
	<OSCPR01MB149662AEA64F4E66F494C2584F5E3A@OSCPR01MB14966.jpnprd01.prod.outlook.com>
	<[email protected]>
	<OS7PR01MB119648846BA926C096CE016B6EAE3A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
	<CAHut+Pt4Tn1bQYCsYeUt_gtcSB-KOTtRB70SLghkpsjfKGsm7w@mail.gmail.com>
	<OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@OS7PR01MB11964.jpnprd01.prod.outlook.com>

HI Iwata-San,

Here are some more review comments for v3.

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

1.
+      <para>
+       <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_DROP</primary></indexterm>
+       Requests to terminate background worker when the database connected by
+       the background worker is changed. DBMS daemon can issue a termination
+       signal to the background worker.
+       This occurs only when significant changes affecting the entire database
+       take place.
+       Specifically, major changes include when the <command>DROP
DATABASE</command>,
+       <command>ALTER DATABASE RENAME TO</command>, and
+       <command>ALTER DATABASE SET TABLESPACE</command> commands are executed.
+      </para>

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>

======


2.
+ for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)
+ {
+ BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno];
+
+ /* Check worker slot. */
+ if (!slot->in_use)
+ continue;
+
+ /* 1st, check cancel flags. */
+ if (slot->worker.bgw_flags & BGWORKER_EXIT_AT_DATABASE_DROP)
+ {
+ PGPROC     *proc = BackendPidGetProc(slot->pid);
+
+ /* 2nd, compare databaseId. */
+ if (proc && proc->databaseId == databaseId)
+ {
+ /*
+ * Set terminate flag in shared memory, unless slot has
+ * been reused.
+ */
+ slot->terminate = true;
+ signal_postmaster = true;
+ }
+ }
+ }


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.

~

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

~

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

~

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.

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

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

view thread (67+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
  In-Reply-To: <CAHut+Pu-nxNice547eGEW2O3hdRcFbPYWF4HiqktZO19X3Ah-g@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox