Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE

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