public inbox for [email protected]
help / color / mirror / Atom feedFrom: Masahiro Ikeda <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: [email protected]
Subject: Re: Support worker_spi to execute the function dynamically.
Date: Thu, 20 Jul 2023 18:08:04 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALj2ACWj7qF-YYVi05okBFJnPugkwsM8PSrEwfHs-tQqKO80Uw@mail.gmail.com>
References: <[email protected]>
<ZLiwP5vrT/[email protected]>
<CALj2ACVOCiXirZFRL-gi7mV2zgdH4e3-vyJQx2fxdzycJTv3qg@mail.gmail.com>
<[email protected]>
<CALj2ACWj7qF-YYVi05okBFJnPugkwsM8PSrEwfHs-tQqKO80Uw@mail.gmail.com>
Hi,
On 2023-07-20 13:50, Bharath Rupireddy wrote:
> On Thu, Jul 20, 2023 at 10:09 AM Michael Paquier <[email protected]>
> wrote:
>>
>> On Thu, Jul 20, 2023 at 09:43:37AM +0530, Bharath Rupireddy wrote:
>> > +1. However, a comment above helps one to understand why some GUCs are
>> > defined before if (!process_shared_preload_libraries_in_progress). As
>> > this is an example extension, it will help understand the reasoning
>> > better. I know we will it in the commit message, but a direct comment
>> > helps:
>> >
>> > /*
>> > * Note that this GUC is defined irrespective of worker_spi shared library
>> > * presence in shared_preload_libraries. It's possible to create the
>> > * worker_spi extension and use functions without it being specified in
>> > * shared_preload_libraries. If we return from here without defining this
>> > * GUC, the dynamic workers launched by worker_spi_launch() will keep
>> > * crashing and restarting.
>> > */
>>
>> WFM to be more talkative here and document things, but I don't think
>> that's it. How about a simple "These GUCs are defined even if this
>> library is not loaded with shared_preload_libraries, for
>> worker_spi_launch()."
>
> LGTM.
Thanks for discussing about the patch. I updated the patch from your
comments
* v2-0001-Support-worker_spi-to-execute-the-function-dynamical.patch
I found another thing to be changed better. Though the tests was assumed
"shared_preload_libraries = worker_spi", the background workers failed
to
be launched in initialized phase because the database is not created
yet.
```
# make check # in src/test/modules/worker_spi
# cat log/postmaster.log # in src/test/modules/worker_spi/
2023-07-20 17:58:47.958 JST worker_spi[853620] FATAL: database
"contrib_regression" does not exist
2023-07-20 17:58:47.958 JST worker_spi[853621] FATAL: database
"contrib_regression" does not exist
2023-07-20 17:58:47.959 JST postmaster[853612] LOG: background worker
"worker_spi" (PID 853620) exited with exit code 1
2023-07-20 17:58:47.959 JST postmaster[853612] LOG: background worker
"worker_spi" (PID 853621) exited with exit code 1
```
It's better to remove "shared_preload_libraries = worker_spi" from the
test configuration. I misunderstood that two background workers would
be launched and waiting at the start of the test.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Attachments:
[text/x-diff] v2-0001-Support-worker_spi-to-execute-the-function-dynamical.patch (3.7K, ../[email protected]/2-v2-0001-Support-worker_spi-to-execute-the-function-dynamical.patch)
download | inline diff:
From 02ef0d8daddd43305842987a6aeac6732b2415a9 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <[email protected]>
Date: Thu, 20 Jul 2023 10:34:50 +0900
Subject: [PATCH] Support worker_spi to execute the function dynamically.
Currently, the database name to connect is initialized only
when process_shared_preload_libraries_in_progress is true.
It means that worker_spi_launch() fails if shared_preload_libraries
is empty because the database to connect is NULL.
The patch changes that the database name is always initilized
when called _PG_init(). We can call worker_spi_launch() and
launch SPI workers dynamically.
It also changes the configuration for the test because we don't
need "shared_preload_libraries = worker_spi" anymore, and
background workers launched in initialized phase always have
failed because the "contrib_regression" database is not created yet.
---
src/test/modules/worker_spi/Makefile | 4 ++--
src/test/modules/worker_spi/dynamic.conf | 3 +--
src/test/modules/worker_spi/worker_spi.c | 27 ++++++++++++++----------
3 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index cbf9b2e37f..c2d32d9b72 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -8,10 +8,10 @@ PGFILEDESC = "worker_spi - background worker example"
REGRESS = worker_spi
-# enable our module in shared_preload_libraries for dynamic bgworkers
REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/worker_spi/dynamic.conf
-# Disable installcheck to ensure we cover dynamic bgworkers.
+# Disable because these tests require "worker_spi.database = contrib_regression",
+# which typical installcheck users do not have (e.g. buildfarm clients).
NO_INSTALLCHECK = 1
ifdef USE_PGXS
diff --git a/src/test/modules/worker_spi/dynamic.conf b/src/test/modules/worker_spi/dynamic.conf
index bfe015f664..d9fd463a53 100644
--- a/src/test/modules/worker_spi/dynamic.conf
+++ b/src/test/modules/worker_spi/dynamic.conf
@@ -1,2 +1 @@
-shared_preload_libraries = worker_spi
-worker_spi.database = contrib_regression
+worker_spi.database = contrib_regression
\ No newline at end of file
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 7227cfaa45..f691fbec9f 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -282,7 +282,12 @@ _PG_init(void)
{
BackgroundWorker worker;
- /* get the configuration */
+ /* Get the configuration */
+
+ /*
+ * These GUCs are defined even if this library is not loaded with
+ * shared_preload_libraries, for worker_spi_launch().
+ */
DefineCustomIntVariable("worker_spi.naptime",
"Duration between each check (in seconds).",
NULL,
@@ -296,6 +301,15 @@ _PG_init(void)
NULL,
NULL);
+ DefineCustomStringVariable("worker_spi.database",
+ "Database to connect to.",
+ NULL,
+ &worker_spi_database,
+ "postgres",
+ PGC_SIGHUP,
+ 0,
+ NULL, NULL, NULL);
+
if (!process_shared_preload_libraries_in_progress)
return;
@@ -312,18 +326,9 @@ _PG_init(void)
NULL,
NULL);
- DefineCustomStringVariable("worker_spi.database",
- "Database to connect to.",
- NULL,
- &worker_spi_database,
- "postgres",
- PGC_POSTMASTER,
- 0,
- NULL, NULL, NULL);
-
MarkGUCPrefixReserved("worker_spi");
- /* set up common data for all our workers */
+ /* Set up common data for all our workers */
memset(&worker, 0, sizeof(worker));
worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
--
2.25.1
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