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
