MIME-Version: 1.0
References: <aOctHMnSZcREh0BR@paquier.xyz>
 <OSCPR01MB1496614832F8014EC16FB78D2F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <aOc7_JGErxvqBLMP@paquier.xyz>
 <OSCPR01MB14966EC12277712131EB8EDF1F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <OS7PR01MB11964C8FE9CDCC0F4C9110988EAEEA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <E8C35CCE-E92F-467B-A604-5C1BBBA8F1C8@gmail.com>
 <OS7PR01MB11964C077A3E61F4887DD3D09EAEFA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <CAHut+PtbOP_80OPZXCUZO=-pBJSRTmHcQ2MnVTFov1meNbw18Q@mail.gmail.com>
 <CAHut+Pt5BN0LDh7OzbNqh9+zqHBgsrLX+vh-gn+3FKYTFHMvhw@mail.gmail.com>
 <TY3PR01MB11969CBD6DF3E0DB820AD4262EAE8A@TY3PR01MB11969.jpnprd01.prod.outlook.com>
 <aPBsd2e2SqLs0Kdk@paquier.xyz>
 <OS7PR01MB119642C7C0E94E767DB7000C1EAE9A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS7PR01MB119642C7C0E94E767DB7000C1EAE9A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Mon, 20 Oct 2025 13:01:31 +1100
Message-ID: 
 <CAHut+PsN3jsKFtsnkwM+0x0Ak4g2dmCaQjb2r=GbX=T+AGRP2w@mail.gmail.com>
Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP
 DATABASE
To: "Aya Iwata (Fujitsu)" <iwata.aya@fujitsu.com>
Cc: Michael Paquier <michael@paquier.xyz>, Chao Li <li.evan.chao@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPsN3jsKFtsnkwM%2B0x0Ak4g2dmCaQjb2r%3DGbX%3DT%2BAGRP2w%40mail.gmail.com>
Precedence: bulk

Hi Iwata-San,

Some comments for the latest v8 patch.

======
src/backend/postmaster/bgworker.c

TerminateBgWorkersByBbOid:

1.
+void
+TerminateBgWorkersByDbOid(Oid oid)

Now the function name is more explicit, but that is not a good reason
to make the parameter name more vague.

IMO the parameter should still be "dbOid" or "databaseId" instead of
just "oid". (ditto for the extern in bgworker.h)

======
src/backend/storage/ipc/procarray.c


CountOtherDBBackends:

2.
+ /*
+ * Usually, we try 50 times with 100ms sleep between tries, making 5 sec
+ * total wait. If requested, it would be reduced to 10 times to shorten the
+ * test time.
+ */


The comment seemed vague to me. How about more like:

/*
 * Retry up to 50 times with 100ms between attempts (max 5s total).
 * Can be reduced to 10 attempts (max 1s total) to speed up tests.
 */

~~~

3.
+ for (tries = 0; tries < ntries; tries++)

'tries' can be declared as a for-loop variable.

~~~

4.
Something feels strange about this function name
(CountOtherDBBackends) which suggests it is just for counting stuff,
but in reality is more about exiting/terminating the workers. In fact
retuns a boolean, not a count. Compare this with this similarly named
"CountUserBackends" which really *is* doing what it says.

Can we give this function a better name, or is that out of scope for this patch?

======
src/test/modules/worker_spi/t/002_worker_terminate.pl

5.
+# Firstly register an injection point to make the test faster. Normally, it
+# spends more than 5 seconds because the backend retries, counting the number
+# of connecting processes 50 times, but now the counting would be done only 10
+# times. See CountOtherDBBackends().
+$node->safe_psql('postgres', "CREATE EXTENSION injection_points;");
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('reduce-ncounts', 'error');");
+

It seemed overkill to give details about what "normally" happens. I
think it is enough to have a simple comment here:

SUGGESTION
The injection point 'reduce-ncounts' reduces the number of backend
retries, allowing for shorter test runs. See CountOtherDBBackends().

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