Re: Support worker_spi to execute the function dynamically.

Hi,

On 2023-07-20 18:39, Bharath Rupireddy wrote:
> On Thu, Jul 20, 2023 at 2:59 PM Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
>>
>> On Thu, Jul 20, 2023 at 05:54:55PM +0900, Masahiro Ikeda wrote:
>> > Yes, you're right. When I tried using worker_spi to test wait event,
>> > I found the behavior. And thanks a lot for your patch. I wasn't aware
>> > of the way. I'll merge your patch to the tests for wait events.
>>
>> Be careful when using that. I have not spent more than a few minutes
>> to show my point, but what I sent lacks a shmem_request_hook in
>> _PG_init(), for example, to request an amount of shared memory equal
>> to the size of the state structure.
>
> I think the preferred way to grab a chunk of shared memory for an
> external module is by using shmem_request_hook and shmem_startup_hook.
> Wait events shared memory too can use them.

OK, I'll add the hooks in worker_spi for the test of wait events.

On 2023-07-21 12:08, Michael Paquier wrote:
> On Thu, Jul 20, 2023 at 03:44:12PM +0530, Bharath Rupireddy wrote:
>> I don't think that change is correct. The worker_spi essentially shows
>> how to start bg workers with RegisterBackgroundWorker and dynamic bg
>> workers with RegisterDynamicBackgroundWorker. If
>> shared_preload_libraries = worker_spi not specified in there, you will
>> miss to start RegisterBackgroundWorkers. Is giving an initidb time
>> database name to worker_spi.database work there? If the database for
>> bg workers doesn't exist, changing bgw_restart_time from
>> BGW_NEVER_RESTART to say 1 will help to see bg workers coming up
>> eventually.
>
> Yeah, it does not move the needle by much. I think that we are
> looking at switching this module to use a TAP test in the long term,
> instead, where it would be possible to test the scenarios we want to
> look at *with* and *without* shared_preload_libraries especially with
> the custom wait events for extensions in mind if we add our tests in
> this module.
>
> It does not change the fact that Ikeda-san is right about the launch
> of dynamic workers with this module being broken, so I have applied v1
> with the comment I have suggested. This will ease a bit the
> implementation of any follow-up test scenarios, while avoiding an
> incorrect pattern in this template module.

Thanks for the commits. As Bharath-san said, I forgot that worker_spi
has an aspect of demonstration and I agree to introduce two types of
tests with and without "shared_preload_libraries = worker_spi".

On 2023-07-21 15:51, Bharath Rupireddy wrote:
> On Fri, Jul 21, 2023 at 11:54 AM Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
>>
>> On Fri, Jul 21, 2023 at 11:24:08AM +0530, Bharath Rupireddy wrote:
>> As we have a dynamic.conf, installcheck is not supported so we don't
>> use anything with this switch. Besides, updating
>> shared_preload_libraries and restarting the node in TAP is cheaper
>> than a second initdb.
>
> In SQL tests, I ensured worker_spi doesn't start static bg workers by
> setting worker_spi.total_workers = 0. Again, all of this is not
> necessary, but it will be a very good example for someone writing
> extensions and play around with custom config files, SQL and TAP tests
> etc.

Thanks for making the patch. I confirmed it works in my environments.

> - snprintf(worker.bgw_name, BGW_MAXLEN, "worker_spi worker %d",
> i);
> - snprintf(worker.bgw_type, BGW_MAXLEN, "worker_spi");
> + snprintf(worker.bgw_name, BGW_MAXLEN, "worker_spi static worker
> %d", i);
> + snprintf(worker.bgw_type, BGW_MAXLEN, "worker_spi static
> worker");
> [..]
> - snprintf(worker.bgw_name, BGW_MAXLEN, "worker_spi worker %d", i);
> - snprintf(worker.bgw_type, BGW_MAXLEN, "worker_spi");
> + snprintf(worker.bgw_name, BGW_MAXLEN, "worker_spi dynamic worker
> %d", i);
> + snprintf(worker.bgw_type, BGW_MAXLEN, "worker_spi dynamic worker");
>
> Good idea to split that.

I agree. It very useful. I'll refer to its implementation for the wait
event tests.

I have some questions about the patch. I'm ok to ignore the following
comment since
your patch is for PoC.

(1)

Do we need to change the minValue from 1 to 0 to support
worker_spi.total_workers = 0?

DefineCustomIntVariable("worker_spi.total_workers",
"Number of workers.",
NULL,
&worker_spi_total_workers,
2,
1,
100,
PGC_POSTMASTER,
0,
NULL,
NULL,
NULL);

(2)

Do we need "worker_spi.total_workers = 0" and
"shared_preload_libraries = worker_spi" in dynamic.conf.

Currently, the static bg workers will not be launched because
"shared_preload_libraries = worker_spi" is removed. So
"worker_spi.total_workers = 0" is meaningless.

(3)

We need change and remove them.

> # Copyright (c) 2021-2023, PostgreSQL Global Development Group
>
> # Test replication statistics data in pg_stat_replication_slots is sane
> after
> # drop replication slot and restart.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION