Hi, Thank you for your comments. I updated patch to v0006.
> -----Original Message----- > From: Peter Smith mailto:[email protected] > Sent: Friday, October 10, 2025 7:57 AM > FYI, I attached a v5 top-up diff for (some of) my above review > comments in case it helps. Thank you very much. I've updated the patch using the attached diff file. > From: Chao Li <[email protected]> > Sent: Friday, October 10, 2025 11:03 AM > 1 - bgworker.sgml > 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” Thank you for your comment an suggested revision. I changed .sgml documentation. >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. Thank you. I fixed this white-spaces (using pgindent). >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) After receiving your comment, I checked other functions and there is no other examples like XXOid function in the code. If this function use only here, original code is using databaseId in argument and it clear what Oid is. I think original name is fine because it's not a function that's called much elsewhere. > 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. Thank you. I fixed this comment. > 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. I changed this function comment: " Terminate all background workers connected to the given database, if they had requested it." Regards, Aya Iwata Fujitsu Limited.
v0006-0001-Allow-background-workers-to-be-terminated.patch
Description: v0006-0001-Allow-background-workers-to-be-terminated.patch
