public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Aya Iwata (Fujitsu) <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE
Date: Mon, 20 Oct 2025 13:01:31 +1100
Message-ID: <CAHut+PsN3jsKFtsnkwM+0x0Ak4g2dmCaQjb2r=GbX=T+AGRP2w@mail.gmail.com> (raw)
In-Reply-To: <OS7PR01MB119642C7C0E94E767DB7000C1EAE9A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
References: <[email protected]>
<OSCPR01MB1496614832F8014EC16FB78D2F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB14966EC12277712131EB8EDF1F5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<OS7PR01MB11964C8FE9CDCC0F4C9110988EAEEA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
<[email protected]>
<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>
<[email protected]>
<OS7PR01MB119642C7C0E94E767DB7000C1EAE9A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
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
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: <CAHut+PsN3jsKFtsnkwM+0x0Ak4g2dmCaQjb2r=GbX=T+AGRP2w@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