public inbox for [email protected]
help / color / mirror / Atom feedFrom: 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: Fri, 10 Oct 2025 09:56:53 +1100
Message-ID: <CAHut+Pu+SMWCCpbwJqq8bqjmD=oA30vnzL1Ey07cik_14KuhDg@mail.gmail.com> (raw)
In-Reply-To: <OS7PR01MB11964C8FE9CDCC0F4C9110988EAEEA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
References: <OS7PR01MB119648846BA926C096CE016B6EAE3A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<CAHut+Pt4Tn1bQYCsYeUt_gtcSB-KOTtRB70SLghkpsjfKGsm7w@mail.gmail.com>
<OS7PR01MB119642CFE4F4DCF7FBBD040BBEAE0A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<CAHut+Pu-nxNice547eGEW2O3hdRcFbPYWF4HiqktZO19X3Ah-g@mail.gmail.com>
<OS7PR01MB11964628A74081DFCA442CBADEAE1A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<CAHut+PuuOKKW2oDT0Z8q+UXsiteS67j6G6075FC3aAxeR0cxHQ@mail.gmail.com>
<OSCPR01MB14966FC50CFB457938763AE09F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<OSCPR01MB14966B98E5119EBB261024ED2F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB1496614832F8014EC16FB78D2F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB14966EC12277712131EB8EDF1F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<OS7PR01MB11964C8FE9CDCC0F4C9110988EAEEA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
Hi Iwata-San,
Some v5 comments.
======
doc/src/sgml/bgworker.sgml
1.
+ <varlistentry>
+ <term><literal>BGWORKER_EXIT_AT_DATABASE_CHANGE</literal></term>
+ <listitem>
+ <para>
+ <indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</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>.
+ When <command>CREATE DATABASE TEMPLATE</command> command is executed,
+ background workers which connected to target template database
are terminated.
+ If <literal>BGWORKER_SHMEM_ACCESS</literal> and
+ <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal> are not using,
+ nothing happens.
+ </para>
+ </listitem>
+ </varlistentry>
+
1a.
The newly added part in v5 needs some brush-up.
SUGGESTION:
<para>
<indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</primary></indexterm>
Requests termination of the background worker when its connected database
undergoes significant changes. 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> (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>
~
1b.
Elsewhere on this docs page, there are detailed discussions about
process termination (e.g. search "terminate"), as well as referring to
the function TerminateBackgroundWorker().
You only documented the new flag, but do we also need to make changes
in the rest of this page to mention the new way for process
termination?
======
src/backend/postmaster/bgworker.c
TerminateBackgroundWorkersByOid:
2.
+/*
+ * Cancel background workers.
+ */
+void
+TerminateBackgroundWorkersByOid(Oid databaseId)
Let's also remove that word "Cancel" from the function comment and add
some more details.
SUGGESTION:
Terminate all background workers connected to the given database, if
they had requested it.
~~~
3.
+ /*
+ * Iterate through slots, looking for workers
+ * who connects to the given database.
+ */
"workers who connects" (??)
SUGGESTION
Iterate through slots, looking for workers connected to the given database.
======
src/backend/storage/ipc/procarray.c
4.
+ /*
+ * Terminate all background workers for this database, if
+ * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP)
+ */
+ TerminateBackgroundWorkersByOid(databaseId);
+
The comment is out-of-date now, because the flag name was changed to
BGWORKER_EXIT_AT_DATABASE_CHANGE.
======
src/include/postmaster/bgworker.h
5.
+/* Cancel background workers. */
+extern void TerminateBackgroundWorkersByOid(Oid databaseId);
There is already a commented extern for the
TerminateBackgroundWorker() function. IMO, just put this extern
alongside that one. You don't need a new comment.
~~~
6. The header comment in this file refers to the
TerminateBackgroundWorker() function. Probably, you need to check that
carefully to see if this new function should also be mentioned there.
======
FYI, I attached a v5 top-up diff for (some of) my above review
comments in case it helps.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
[application/octet-stream] PS_v5_topup.diff (3.9K, 2-PS_v5_topup.diff)
download | inline diff:
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 01ceb38..d6e37b5 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -113,17 +113,16 @@ typedef struct BackgroundWorker
<listitem>
<para>
<indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</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>.
- When <command>CREATE DATABASE TEMPLATE</command> command is executed,
- background workers which connected to target template database are terminated.
- If <literal>BGWORKER_SHMEM_ACCESS</literal> and
- <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal> are not using,
- nothing happens.
+ Requests termination of the background worker when its connected database
+ undergoes significant changes. 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> (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>
</listitem>
</varlistentry>
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 7ab48c2..e0cfdbf 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -1400,7 +1400,8 @@ GetBackgroundWorkerTypeByPid(pid_t pid)
/*
- * Cancel background workers.
+ * Terminate all background workers connected to the given database, if they
+ * had requested it.
*/
void
TerminateBackgroundWorkersByOid(Oid databaseId)
@@ -1410,8 +1411,8 @@ TerminateBackgroundWorkersByOid(Oid databaseId)
LWLockAcquire(BackgroundWorkerLock, LW_EXCLUSIVE);
/*
- * Iterate through slots, looking for workers
- * who connects to the given database.
+ * Iterate through slots, looking for workers connected to the given
+ * database.
*/
for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; ++slotno)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index f612e77..1ed641d 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3771,7 +3771,7 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
/*
* Terminate all background workers for this database, if
- * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP)
+ * they had requested it (BGWORKER_EXIT_AT_DATABASE_CHANGE)
*/
TerminateBackgroundWorkersByOid(databaseId);
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 57741d7..3a2fc35 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -134,6 +134,7 @@ extern const char *GetBackgroundWorkerTypeByPid(pid_t pid);
/* Terminate a bgworker */
extern void TerminateBackgroundWorker(BackgroundWorkerHandle *handle);
+extern void TerminateBackgroundWorkersByOid(Oid databaseId);
/* This is valid in a running worker */
extern PGDLLIMPORT BackgroundWorker *MyBgworkerEntry;
@@ -167,7 +168,4 @@ extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, ui
extern void BackgroundWorkerBlockSignals(void);
extern void BackgroundWorkerUnblockSignals(void);
-/* Cancel background workers. */
-extern void TerminateBackgroundWorkersByOid(Oid databaseId);
-
#endif /* BGWORKER_H */
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+SMWCCpbwJqq8bqjmD=oA30vnzL1Ey07cik_14KuhDg@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