Subject: Support worker_spi to execute the function dynamically.

From: Masahiro Ikeda <[email-masked]>
Date: Thu, 20 Jul 2023 11:15:51 +0900

Hi,

While I'm working on the thread[1], I found that the function of
worker_spi module fails if 'shared_preload_libraries' doesn't have
worker_spi.

The reason is that the database name is NULL because the database name
is initialized only when process_shared_preload_libraries_in_progress
is true.

```
psql=# SELECT worker_spi_launch(1) ;
2023-07-20 11:00:56.491 JST [1179891] LOG:  worker_spi worker 1 
initialized with schema1.counted
2023-07-20 11:00:56.491 JST [1179891] FATAL:  cannot read pg_class 
without having selected a database at character 22
2023-07-20 11:00:56.491 JST [1179891] QUERY:  select count(*) from 
pg_namespace where nspname = 'schema1'
2023-07-20 11:00:56.491 JST [1179891] STATEMENT:  select count(*) from 
pg_namespace where nspname = 'schema1'
2023-07-20 11:00:56.492 JST [1179095] LOG:  background worker 
"worker_spi" (PID 1179891) exited with exit code 1
```

In my understanding, the restriction is not required. So, I think it's
better to change the behavior.
(v1-0001-Support-worker_spi-to-execute-the-function-dynamical.patch)

What do you think?

[1] Support to define custom wait events for extensions
https://www.postgresql.org/message-id/flat/b9f5411acda0cf15c8fbb767702ff43e%40oss.nttdata.com

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

From c6e60c66c4066b4a01981ffae5a168901e7283eb Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <[email-masked]>
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.
---
 src/test/modules/worker_spi/worker_spi.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 7227cfaa45..ccc38a36d3 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -296,6 +296,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,15 +321,6 @@ _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 */
-- 
2.25.1

---

From: Michael Paquier <[email-masked]>
Date: Thu, 20 Jul 2023 12:55:43 +0900

> worker_spi module fails if 'shared_preload_libraries' doesn't have

I guess that you were patching worker_spi to register dynamically a
wait event and embed that in a TAP test or similar without loading it
in shared_preload_libraries?  FWIW, you could use a trick like what I
am attaching here to load a wait event dynamically with the custom
wait event API.  You would need to make worker_spi_init_shmem() a bit
more aggressive with an extra hook to reserve a shmem area size, but
that's enough to show the custom wait event in the same backend as the
one that launches a worker_spi dynamically, while demonstrating how
the API can be used in this case.

> (v1-0001-Support-worker_spi-to-execute-the-function-dynamical.patch)

+1.  I'm OK to lift this restriction with a SIGHUP GUC for the
database name and that's not a pattern to encourage in a template
module.  Will do so, if there are no objections.
--
Michael
From 2fcd773cd4009cffd626640e1553d4efdceb0777 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 20 Jul 2023 12:48:07 +0900
Subject: [PATCH] Add tweak to allow worker_spi to register wait_event
 dynamically

---
 src/test/modules/worker_spi/worker_spi.c | 35 +++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index b87d6c75d8..4e3da93129 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -48,11 +48,20 @@ PG_FUNCTION_INFO_V1(worker_spi_launch);
 
 PGDLLEXPORT void worker_spi_main(Datum main_arg) pg_attribute_noreturn();
 
+static void worker_spi_init_shmem(void);
+
 /* GUC variables */
 static int	worker_spi_naptime = 10;
 static int	worker_spi_total_workers = 2;
 static char *worker_spi_database;
 
+typedef struct WorkerSpiState
+{
+	uint32 wait_event_info;
+} WorkerSpiState;
+
+/* the pointer to the shared memory */
+static WorkerSpiState *worker_spi_state = NULL;
 
 typedef struct worktable
 {
@@ -149,6 +158,8 @@ worker_spi_main(Datum main_arg)
 	/* We're now ready to receive signals */
 	BackgroundWorkerUnblockSignals();
 
+	worker_spi_init_shmem();
+
 	/* Connect to our database */
 	BackgroundWorkerInitializeConnection(worker_spi_database, NULL, 0);
 
@@ -199,7 +210,7 @@ worker_spi_main(Datum main_arg)
 		(void) WaitLatch(MyLatch,
 						 WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
 						 worker_spi_naptime * 1000L,
-						 WAIT_EVENT_EXTENSION);
+						 worker_spi_state->wait_event_info);
 		ResetLatch(MyLatch);
 
 		CHECK_FOR_INTERRUPTS();
@@ -346,6 +357,26 @@ _PG_init(void)
 	}
 }
 
+static void
+worker_spi_init_shmem(void)
+{
+	bool found = false;
+
+	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+	worker_spi_state = ShmemInitStruct("worker_spi",
+									   sizeof(WorkerSpiState),
+									   &found);
+	if (!found)
+	{
+		/* First time through ... */
+		worker_spi_state->wait_event_info = WaitEventExtensionNew();
+	}
+	LWLockRelease(AddinShmemInitLock);
+
+	WaitEventExtensionRegisterName(worker_spi_state->wait_event_info,
+								   "worker_spi_custom");
+}
+
 /*
  * Dynamically launch an SPI worker.
  */
@@ -358,6 +389,8 @@ worker_spi_launch(PG_FUNCTION_ARGS)
 	BgwHandleStatus status;
 	pid_t		pid;
 
+	worker_spi_init_shmem();
+
 	memset(&worker, 0, sizeof(worker));
 	worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
 		BGWORKER_BACKEND_DATABASE_CONNECTION;
-- 
2.40.1

---

From: Masahiro Ikeda <[email-masked]>
Date: Thu, 20 Jul 2023 17:54:55 +0900

> the API can be used in this case.

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.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

---

From: Michael Paquier <[email-masked]>
Date: Thu, 20 Jul 2023 18:29:10 +0900

> 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.
--
Michael

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 20 Jul 2023 15:09:04 +0530

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

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com