Re: Support worker_spi to execute the function dynamically.

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Masahiro Ikeda <[email protected]>
To: Bharath Rupireddy <[email protected]>
To: [email protected]
Cc: [email protected]
Subject: Re: Support worker_spi to execute the function dynamically.
Date: Fri, 21 Jul 2023 19:35:36 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALj2ACVWW2NU78kJ=xEbz8qcZgWeae+cVp8TYbvGTdpYjVKjjA@mail.gmail.com>
References: <[email protected]>
	<ZLiwP5vrT/[email protected]>
	<CALj2ACVOCiXirZFRL-gi7mV2zgdH4e3-vyJQx2fxdzycJTv3qg@mail.gmail.com>
	<[email protected]>
	<CALj2ACWj7qF-YYVi05okBFJnPugkwsM8PSrEwfHs-tQqKO80Uw@mail.gmail.com>
	<[email protected]>
	<CALj2ACV0fr-9OTZqtWu-9XrtUbBZCYb0UK859gfvJ0oqm3YvUA@mail.gmail.com>
	<ZLn2v/[email protected]>
	<CALj2ACVUxgcQaGh3HpBJ6Xg9FRqj7UNDomd5E1y=tRKs2wcntA@mail.gmail.com>
	<[email protected]>
	<CALj2ACVWW2NU78kJ=xEbz8qcZgWeae+cVp8TYbvGTdpYjVKjjA@mail.gmail.com>

Hi,

On 2023-07-20 18:39, Bharath Rupireddy wrote:
> On Thu, Jul 20, 2023 at 2:59 PM Michael Paquier <[email protected]> 
> 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 <[email protected]> 
> 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

view thread (18+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Support worker_spi to execute the function dynamically.
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox