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/
