BUG #16914: Regression test of the worker_spi fails if USE_MODULE_DB environment variable is set.

On Thu, Mar 04, 2021 at 07:43:11AM +0000, PG Bug reporting form wrote:

I found a little problem with the test on the master branch:

$ make -s -C src/test/modules/worker_spi check USE_MODULE_DB=TRUE
============== running regression test queries ==============
test worker_spi ... FAILED 131652 ms
============== shutting down postmaster

Fun.

Something like this fixes the problem:

One problem with this solution is that this is not portable. Instead
of generating dynamically the configuration file, here is funkier
solution as we know that worker_spi would restart if the database it
should connect to does not exist: let's use a completely different
database name, not expected by any other modules. Then, this database
gets created in worker_spi.sql, with a reconnection to it.
--
Michael

On 2021-Mar-04, Michael Paquier wrote:

test worker_spi ... FAILED 131652 ms
============== shutting down postmaster

Fun.

Eek.

Something like this fixes the problem:

One problem with this solution is that this is not portable. Instead
of generating dynamically the configuration file, here is funkier
solution as we know that worker_spi would restart if the database it
should connect to does not exist: let's use a completely different
database name, not expected by any other modules. Then, this database
gets created in worker_spi.sql, with a reconnection to it.

It's not that I *like* this solution, but let's say that I dislike it
less than the other one. If there are no other ideas, I +1 this one.

--
�lvaro Herrera 39�49'30"S 73�17'W

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-Mar-04, Michael Paquier wrote:

One problem with this solution is that this is not portable. Instead
of generating dynamically the configuration file, here is funkier
solution as we know that worker_spi would restart if the database it
should connect to does not exist: let's use a completely different
database name, not expected by any other modules. Then, this database
gets created in worker_spi.sql, with a reconnection to it.

It's not that I *like* this solution, but let's say that I dislike it
less than the other one. If there are no other ideas, I +1 this one.

The real problem here is that worker_spi illustrates a fundamentally
useless way to define which database a worker ought to connect to.
It's basically impossible for it to use anything except the hard-wired
default for the worker_spi.database GUC. You could wish that we could
fix it like

 CREATE EXTENSION worker_spi;
+SELECT set_config('worker_spi.database', current_database(), false);
 SELECT worker_spi_launch(4) IS NOT NULL;
 -- wait until the worker completes its initialization

but that doesn't work because the session-local setting of the GUC won't
be seen over in the worker process.

Since the alleged point of this module is to be a prototype for
useful background workers, I think we should think of another way.
Maybe we need to work a bit harder on passing values from
worker_spi_launch to the worker, so that the DB name could be a
parameter to worker_spi_launch.

regards, tom lane

On Thu, Mar 04, 2021 at 05:14:57PM -0500, Tom Lane wrote:

CREATE EXTENSION worker_spi;
+SELECT set_config('worker_spi.database', current_database(), false);
SELECT worker_spi_launch(4) IS NOT NULL;
-- wait until the worker completes its initialization

but that doesn't work because the session-local setting of the GUC won't
be seen over in the worker process.

Since the alleged point of this module is to be a prototype for
useful background workers, I think we should think of another way.
Maybe we need to work a bit harder on passing values from
worker_spi_launch to the worker, so that the DB name could be a
parameter to worker_spi_launch.

Yeah, or this is converted to a TAP test where we don't care about
the database used, but that would consume more cycles without more
coverage done. Don't we need to worry about the memory context where
bgw_main_arg is pushed into for dynamic workers?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Thu, Mar 04, 2021 at 05:14:57PM -0500, Tom Lane wrote:

Since the alleged point of this module is to be a prototype for
useful background workers, I think we should think of another way.
Maybe we need to work a bit harder on passing values from
worker_spi_launch to the worker, so that the DB name could be a
parameter to worker_spi_launch.

Yeah, or this is converted to a TAP test where we don't care about
the database used, but that would consume more cycles without more
coverage done. Don't we need to worry about the memory context where
bgw_main_arg is pushed into for dynamic workers?

Yeah, the fact that bgw_main_arg is declared as Datum is really kind
of a lie, because the only thing that actually works there is a pass
by value datatype (not that our docs tell you so).

We could stuff the database name into bgw_extra perhaps, but that
seems restrictive as well. I wonder if there's a reasonable way to
pass over a struct of caller-defined size.

regards, tom lane

On Fri, Mar 05, 2021 at 12:42:43AM -0500, Tom Lane wrote:

Yeah, the fact that bgw_main_arg is declared as Datum is really kind
of a lie, because the only thing that actually works there is a pass
by value datatype (not that our docs tell you so).

Exactly.

We could stuff the database name into bgw_extra perhaps, but that
seems restrictive as well. I wonder if there's a reasonable way to
pass over a struct of caller-defined size.

Or we could just rely on a custom memory context that dynamic workers
could use to store their data? It could be simpler IMO to just merge
together bgw_main_arg and bgw_extra, then pass down the whole as an
argument of the main routine of the bgworker, say with a buffer of 256
bytes for example. I am not sure that workers really need that much
for themselves at startup out of GUCs, but I may be wrong :)
--
Michael

On 2021-Mar-05, Michael Paquier wrote:

Or we could just rely on a custom memory context that dynamic workers
could use to store their data? It could be simpler IMO to just merge
together bgw_main_arg and bgw_extra, then pass down the whole as an
argument of the main routine of the bgworker, say with a buffer of 256
bytes for example.

I think the only way to make that work portably is by using shared memory.

--
�lvaro Herrera Valdivia, Chile
"La rebeld�a es la virtud original del hombre" (Arthur Schopenhauer)