Subject: Re: Support worker_spi to execute the function dynamically.

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

On Thu, Jul 20, 2023 at 9:25 AM Michael Paquier <[email-masked]> wrote:
>
> > 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.  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.

+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.
     */

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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 20 Jul 2023 13:39:36 +0900

>      * 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()."
--
Michael

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 20 Jul 2023 10:20:43 +0530

> library is not loaded with shared_preload_libraries, for

LGTM.

---

From: Masahiro Ikeda <[email-masked]>
Date: Thu, 20 Jul 2023 18:08:04 +0900

Hi,

>> library is not loaded with shared_preload_libraries, for

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

From 02ef0d8daddd43305842987a6aeac6732b2415a9 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.

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

---

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

> be launched and waiting at the start of the test.

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.

I think it's worth adding test cases for the expected number of bg
workers (after creating worker_spi extension) and dynamic bg workers
(after calling worker_spi_launch()). Also, to distinguish bg workers
and dynamic bg workers, you can change
bgw_type in worker_spi_launch to "worker_spi dynamic worker".

These unrelated changes better be there as-is. Because, the postgres
code has both commenting styles /* Get .... */ or /* get ....*/, IOW,
single line comments starting with both uppercase and lowercase.

---

From: Michael Paquier <[email-masked]>
Date: Fri, 21 Jul 2023 12:08:47 +0900

> BGW_NEVER_RESTART to say 1 will help to see bg workers coming up

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

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 21 Jul 2023 11:24:08 +0530

> the custom wait events for extensions in mind if we add our tests in

Okay. Here's a quick patch for adding TAP tests to the worker_spi
module. We can change it to taste.

Thoughts?

---

From: Michael Paquier <[email-masked]>
Date: Fri, 21 Jul 2023 15:24:19 +0900

> module. We can change it to taste.

What do you think if we removed completely the sql/ test, moving it to
TAP so as we have only one cluster set up when running a make check?
worker_spi.sql only does two waits (one for the initialization and one
to check that the tuple has been processed), so these could be
replaced by some poll_query_until()?

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.

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

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 21 Jul 2023 12:21:57 +0530

> replaced by some poll_query_until()?

I think we can keep SQL tests around as it will help demonstrate
someone quickly write their own SQL tests.

> shared_preload_libraries and restarting the node in TAP is cheaper

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.

---

From: Masahiro Ikeda <[email-masked]>
Date: Fri, 21 Jul 2023 19:35:36 +0900

Hi,

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

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

> extensions and play around with custom config files, SQL and TAP tests

Thanks for making the patch. I confirmed it works in my environments.

> +   snprintf(worker.bgw_type, BGW_MAXLEN, "worker_spi dynamic worker");

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

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 21 Jul 2023 21:35:06 +0530

> Thanks for making the patch. I confirmed it works in my environments.

Thanks for verifying.

> I have some questions about the patch.
>
> (1)
>

No, let's keep it that way.

> "worker_spi.total_workers = 0" is meaningless.

You're right. worker_spi.total_workers = 0 in custom.conf has no
effect. without shared_preload_libraries = worker_spi. Removed that.

> > # drop replication slot and restart.

Modified.

I'm attaching the v2 patch. Thoughts?

---

From: Masahiro Ikeda <[email-masked]>
Date: Mon, 24 Jul 2023 10:04:39 +0900

> effect. without shared_preload_libraries = worker_spi. Removed that.

OK. If so, we need to remove the following comment in Makefile.

> # enable our module in shared_preload_libraries for dynamic bgworkers

I also confirmed that the tap tests work with meson and make.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 24 Jul 2023 08:31:01 +0530

> > # enable our module in shared_preload_libraries for dynamic bgworkers

Done.

Thanks for verifying.

I also added a note atop worker_spi.c that the module also
demonstrates how to write core (SQL) tests and extended (TAP) tests.

I'm attaching the v3 patch.

---

From: Masahiro Ikeda <[email-masked]>
Date: Mon, 24 Jul 2023 16:26:45 +0900

> I'm attaching the v3 patch.

I verified it works and it looks good to me.
Thanks to your work, I will be able to implement tests for
custom wait events.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

---

From: Andres Freund <[email-masked]>
Date: Thu, 27 Jul 2023 19:23:32 -0700

Hi,

The new test fails with my AIO branch occasionally. But I'm fairly certain
that's just due to timing differences.

Excerpt from the log:

2023-07-27 21:43:00.385 UTC [42339] LOG:  worker_spi worker 3 initialized with schema3.counted
2023-07-27 21:43:00.399 UTC [42344] 001_worker_spi.pl LOG:  statement: SELECT datname, count(datname) FROM pg_stat_activity
	            WHERE backend_type = 'worker_spi' GROUP BY datname;
2023-07-27 21:43:00.403 UTC [42340] LOG:  worker_spi worker 2 initialized with schema2.counted
2023-07-27 21:43:00.407 UTC [42341] LOG:  worker_spi worker 1 initialized with schema1.counted
2023-07-27 21:43:00.420 UTC [42346] 001_worker_spi.pl LOG:  statement: SELECT worker_spi_launch(1);
2023-07-27 21:43:00.423 UTC [42347] LOG:  worker_spi dynamic worker 1 initialized with schema1.counted
2023-07-27 21:43:00.432 UTC [42349] 001_worker_spi.pl LOG:  statement: SELECT worker_spi_launch(2);
2023-07-27 21:43:00.437 UTC [42350] LOG:  worker_spi dynamic worker 2 initialized with schema2.counted
2023-07-27 21:43:00.443 UTC [42347] ERROR:  duplicate key value violates unique constraint "pg_namespace_nspname_index"
2023-07-27 21:43:00.443 UTC [42347] DETAIL:  Key (nspname)=(schema1) already exists.
2023-07-27 21:43:00.443 UTC [42347] CONTEXT:  SQL statement "CREATE SCHEMA "schema1" CREATE TABLE "counted" (		type text CHECK (type IN ('total', 'delta')), 		value	integer)CREATE UNIQUE INDEX "counted_unique_total" ON "counted" (type) WHERE type = 'total'"

As written, dynamic and static workers race each other. It doesn't make a lot
of sense to me to use the same ids for either?

The attached patch reproduces the problem on master.

Note that without the sleep(3) in the test the workers don't actually finish
starting, the test shuts down the cluster before that happens...

Greetings,

Andres Freund
diff --git i/src/test/modules/worker_spi/t/001_worker_spi.pl w/src/test/modules/worker_spi/t/001_worker_spi.pl
index c2938713134..093a038b005 100644
--- i/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ w/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -52,6 +52,7 @@ $node->append_conf(
 shared_preload_libraries = 'worker_spi'
 worker_spi.database = 'mydb'
 worker_spi.total_workers = 3
+log_statement=all
 });
 $node->restart;
 
@@ -68,6 +69,9 @@ ok( $node->poll_query_until(
 # check their existence.
 my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(1);');
 my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(2);');
+
+sleep(3);
+
 ok( $node->poll_query_until(
 		'mydb',
 		qq[SELECT datname, count(datname)  FROM pg_stat_activity
diff --git i/src/test/modules/worker_spi/worker_spi.c w/src/test/modules/worker_spi/worker_spi.c
index 903dcddef97..ebd2120ec11 100644
--- i/src/test/modules/worker_spi/worker_spi.c
+++ w/src/test/modules/worker_spi/worker_spi.c
@@ -120,6 +120,8 @@ initialize_worker_spi(worktable *table)
 			elog(FATAL, "failed to create my schema");
 
 		debug_query_string = NULL;	/* rest is not statement-specific */
+
+		pg_usleep(USECS_PER_SEC*1);
 	}
 
 	SPI_finish();
@@ -127,6 +129,8 @@ initialize_worker_spi(worktable *table)
 	CommitTransactionCommand();
 	debug_query_string = NULL;
 	pgstat_report_activity(STATE_IDLE, NULL);
+
+	elog(LOG, "done");
 }
 
 void

---

From: Michael Paquier <[email-masked]>
Date: Fri, 28 Jul 2023 13:45:29 +0900

> starting, the test shuts down the cluster before that happens...

So you have faced a race condition where the commit of the transaction
doing the schema creation for the static workers is delayed long
enough that the dynamic workers don't see it, and bumped on a catalog
conflict when they try to create the same schemas.

Having each bgworker on its own schema would be enough to prevent
conflicts, but I'd like to add a second thing: a check on
pg_stat_activity.wait_event after starting the workers.  I have added
something like that in the patch I have posted today for the custom
wait events at [1] and it enforces the startup sequences of the
workers in a stricter way.

Does the attached take care of your issue?

[1]: https://www.postgresql.org/message-id/[email-masked]
--
Michael
diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
index c293871313..f370ba93b8 100644
--- a/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -58,22 +58,24 @@ $node->restart;
 # Check that bgworkers have been registered and launched.
 ok( $node->poll_query_until(
 		'mydb',
-		qq[SELECT datname, count(datname) FROM pg_stat_activity
-            WHERE backend_type = 'worker_spi' GROUP BY datname;],
-		'mydb|3'),
+		qq[SELECT datname, wait_event, count(datname) FROM pg_stat_activity
+            WHERE backend_type = 'worker_spi' GROUP BY datname, wait_event;],
+		'mydb|Extension|3'),
 	'bgworkers all launched'
 ) or die "Timed out while waiting for bgworkers to be launched";
 
 # Ask worker_spi to launch dynamic bgworkers with the library loaded, then
-# check their existence.
-my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(1);');
-my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(2);');
+# check their existence.  Use IDs that do not overlap with the schemas created
+# by the previous workers.
+my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(10);');
+my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(11);');
+
 ok( $node->poll_query_until(
 		'mydb',
-		qq[SELECT datname, count(datname)  FROM pg_stat_activity
+		qq[SELECT datname, wait_event, count(datname)  FROM pg_stat_activity
             WHERE backend_type = 'worker_spi dynamic' AND
-            pid IN ($worker1_pid, $worker2_pid) GROUP BY datname;],
-		'mydb|2'),
+            pid IN ($worker1_pid, $worker2_pid) GROUP BY datname, wait_event;],
+		'mydb|Extension|2'),
 	'dynamic bgworkers all launched'
 ) or die "Timed out while waiting for dynamic bgworkers to be launched";

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 28 Jul 2023 10:47:39 +0530

While using different IDs in tests is a simple fix, -1 for it. I'd
prefer if worker_spi uses different schema prefixes for static and
dynamic bg workers to avoid conflicts. We can either look at
MyBgworkerEntry->bgw_type in worker_spi_main and have schema name as
'{static, dyamic}_worker_schema_%d', id or pass schema name in
bgw_extra.