MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Fri, 21 Jul 2023 19:35:36 +0900
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 michael@paquier.xyz
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Support worker_spi to execute the function dynamically.
In-Reply-To: <CALj2ACVWW2NU78kJ=xEbz8qcZgWeae+cVp8TYbvGTdpYjVKjjA@mail.gmail.com>
References: <d30d3ea7d21cb7c9e1e3cc47e301f1b6@oss.nttdata.com>
 <ZLiwP5vrT/SENzim@paquier.xyz>
 <CALj2ACVOCiXirZFRL-gi7mV2zgdH4e3-vyJQx2fxdzycJTv3qg@mail.gmail.com>
 <ZLi6iHwMAqhfYVdn@paquier.xyz>
 <CALj2ACWj7qF-YYVi05okBFJnPugkwsM8PSrEwfHs-tQqKO80Uw@mail.gmail.com>
 <46e53055d8d1c92704d3cd2709c722e7@oss.nttdata.com>
 <CALj2ACV0fr-9OTZqtWu-9XrtUbBZCYb0UK859gfvJ0oqm3YvUA@mail.gmail.com>
 <ZLn2v/eMkqmny4J6@paquier.xyz>
 <CALj2ACVUxgcQaGh3HpBJ6Xg9FRqj7UNDomd5E1y=tRKs2wcntA@mail.gmail.com>
 <ZLokkzw6pmvDU9bM@paquier.xyz>
 <CALj2ACVWW2NU78kJ=xEbz8qcZgWeae+cVp8TYbvGTdpYjVKjjA@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <e4e5bee9d241a00a0edd9ee4e90fe69c@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/e4e5bee9d241a00a0edd9ee4e90fe69c%40oss.nttdata.com>
Precedence: bulk

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