Hi,
On 2023-07-20 18:39, Bharath Rupireddy wrote:
On Thu, Jul 20, 2023 at 2:59 PM Michael Paquier <mich...@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 <mich...@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
