Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE - Mailing list pgsql-hackers

Hi Iwata-San.

It is not at all clear to me if you are advocating for the flag to be
called INTERRUPTABLE or to be called PROTECTED?

IIUC, behaviour-wise it ultimately ends up the same, just the flags
are different names with opposite meanings and defaults. Still, you
need to choose ASAP because this decision touches a lot of code,
comments and tests.

It seems that when Michael wrote, there were "more advantages in ...
make a bgworker interruptible an opt-in choice" [1], he is favouring
keeping it as the INTERRUPTABLE flag -- e.g. discard patch 0002. Am I
reading this thread correctly?

~~~

Anyway, just in case you are PROTECTED is still under consideration,
then below are some review comments for the v11-0002 patch.

======
doc/src/sgml/bgworker.sgml

1a.
The commit message says that "default behavior to Terminate." which I
assumed means that by default, the PROTECTED is *note* set. But the
default behaviour is not mentioned in the SGML help?

1b.
Also, the INTERRUPTABLE flag was documented that it required both
BGWORKER_SHMEM_ACCESS and BGWORKER_BACKEND_DATABASE_CONNECTION as
well. But did it make sense to document that those are both still
required for PROTECTED?

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

2.
TerminateInterruptableBgWorkersByDbOid:

AFAIK, you only called the function this name because of the
INTERRUPTABLE flag. But, if you are going to use PROTECTED instead,
then the function name should likewise change to something like
TerminateUnprotectedBgWorkersByDbOid.

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

CountOtherDBBackends:

3.
  /*
  * Terminate all background workers for this database, if they had
- * requested it (BGWORKER_INTERRUPTABLE)
+ * requested it (BGWORKER_PROTECTED)
  */

The comment seems wrong because BGWORKER_PROTECTED means do NOT
terminate it because it is protected. So the comment should be more
like:
Terminate all background workers for this database, unless they are
flagged as BGWORKER_PROTECTED.

======
src/include/postmaster/bgworker.h

4.
-#define BGWORKER_INTERRUPTABLE 0x0004
+#define BGWORKER_PROTECTED 0x0004

You cannot change the name to have the opposite meaning, without also
rewriting the preceding code comment.

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

5.
-# Ensure CREATE DATABASE WITH TEMPLATE fails because background worker retains
+# Ensure CREATE DATABASE WITH TEMPLATE sucseeds because background
worker retains

Typo - "sucseeds"
Typo - "background worker retains" ?? Do you mean "remains" ??

~~~

6.
The code below:
    my $result = $node->safe_psql(
        $database, qq(
        SELECT worker_spi_launch($testcase, oid, 0, '{}',
$request_terminate) IS NOT NULL
        FROM pg_database WHERE datname = '$database';
    ));
...

seems contradictary because the 4th parameter ($request_terminate) of
worker_spi_launch() was changed to *prevent* termination, not request
it.

e.g.
bool prevent_termination = PG_GETARG_BOOL(4);

It makes me doubtful about the testcode validity/correctness, given
that the parameter now has the opposite meaning from patch 0001, but
still the test is passing.

======
[1] https://www.postgresql.org/message-id/aVHQt-asgKx5dnap%40paquier.xyz

Kind Regards,
Peter Smith.
Fujitsu Australia