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@paquier.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@paquier.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