public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Aya Iwata (Fujitsu) <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
Date: Fri, 10 Oct 2025 10:03:06 +0800
Message-ID: <[email protected]> (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,
A few comments:
> On Oct 9, 2025, at 21:09, Aya Iwata (Fujitsu) <[email protected]> wrote:
>
> Hi,
>
> I updated the patch to v0005.
>
> Regards,
> Aya Iwata
> Fujitsu Limited
> <v0005-0001-Allow-background-workers-to-be-terminated.patch>
1 - bgworker.sgml
```
+ <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>
```
This paragraph has several English problems:
* “Undergoes significant changes” sounds vague, better to say “is dropped, renamed or moved to a different tablespace”.
* “When CREATE DATABASE TEMPLATE command is executed” - missing articles.
* “background workers which connected to target template database” - wrong tense/relative pronoun.
* “are not using” should be “are not used” or “are not set”
Suggested revision:
```
<indexterm><primary>BGWORKER_EXIT_AT_DATABASE_CHANGE</primary></indexterm>
Requests termination of the background worker when the database it is
connected to is dropped, renamed, or moved to a different tablespace.
In these cases, 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 a <command>CREATE DATABASE ... TEMPLATE ...</command> command is executed,
background workers connected to the template database used as the source are
also terminated.
If neither <literal>BGWORKER_SHMEM_ACCESS</literal> nor
<literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal> is set, this action
has no effect.
```
2 - bgworker.h
```
+#define BGWORKER_EXIT_AT_DATABASE_CHANGE 0x0004
```
You are using white-spaces between the macro name and value, that’s why 0x0004 looks not aligned in my IDE. I think you should use a couple tabs between them.
3 - bgworker.h
```
+extern void TerminateBackgroundWorkersByOid(Oid databaseId);
```
An OID can represent a lot of things. So, instead of suggesting the OID type by parameter name, I wonder if it is better do that with the function name, like TerminateBgWorkersByDbOid(Oid oid)
4 - procarray.c
```
+ /*
+ * Terminate all background workers for this database, if
+ * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP)
+ */
+ TerminateBackgroundWorkersByOid(databaseId);
```
I wonder if the correct parameter should be BGWORKER_EXIT_AT_DATABASE_CHANGE in the comment, as you are adding BGWORKER_EXIT_AT_DATABASE_CHANGE with this patch.
5 - bgworker.c
```
+/*
+ * Cancel background workers.
+ */
+void
+TerminateBackgroundWorkersByOid(Oid databaseId)
```
I think the function name is more descriptive than the function comment. So, please either remove function comment or enhance it.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
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], [email protected]
Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
In-Reply-To: <[email protected]>
* 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