public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/7] Write WAL for empty nbtree index build
18+ messages / 5 participants
[nested] [flat]
* [PATCH 2/7] Write WAL for empty nbtree index build
@ 2018-10-11 01:03 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Kyotaro Horiguchi @ 2018-10-11 01:03 UTC (permalink / raw)
After relation truncation indexes are also rebuild. It doesn't emit
WAL in minimal mode and if truncation happened within its creation
transaction, crash recovery leaves an empty index heap, which is
considered broken. This patch forces to emit WAL when an index_build
turns into empty nbtree index.
---
src/backend/access/nbtree/nbtsort.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 14d9545768..5551a9c227 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -622,8 +622,16 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
/* Ensure rd_smgr is open (could have been closed by relcache flush!) */
RelationOpenSmgr(wstate->index);
- /* XLOG stuff */
- if (wstate->btws_use_wal)
+ /* XLOG stuff
+ *
+ * Even when wal_level is minimal, WAL is required here if truncation
+ * happened after being created in the same transaction. This is hacky but
+ * we cannot use BufferNeedsWAL() stuff for nbtree since it can emit
+ * atomic WAL records on multiple buffers.
+ */
+ if (wstate->btws_use_wal ||
+ (RelationNeedsWAL(wstate->index) &&
+ (blkno == BTREE_METAPAGE && BTPageGetMeta(page)->btm_root == 0)))
{
/* We use the heap NEWPAGE record type for this */
log_newpage(&wstate->index->rd_node, MAIN_FORKNUM, blkno, page, true);
--
2.16.3
----Next_Part(Tue_Apr_02_19_54_06_2019_801)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v10-0003-Move-XLOG-stuff-from-heap_insert-and-heap_delete.patch"
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
@ 2023-07-20 04:13 Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-20 04:13 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
On Thu, Jul 20, 2023 at 9:25 AM Michael Paquier <[email protected]> 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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-20 04:39 ` Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2023-07-20 04:39 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
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()."
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
@ 2023-07-20 04:50 ` Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-20 04:50 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
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.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-20 09:08 ` Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Masahiro Ikeda @ 2023-07-20 09:08 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
@ 2023-07-20 10:14 ` Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-20 10:14 UTC (permalink / raw)
To: Masahiro Ikeda <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]
On Thu, Jul 20, 2023 at 2:38 PM Masahiro Ikeda <[email protected]> wrote:
>
> 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.
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".
- /* get the configuration */
+ /* Get the configuration */
- /* set up common data for all our workers */
+ /* Set up common data for all our workers */
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.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-21 03:08 ` Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2023-07-21 03:08 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
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.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZLn2v%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
@ 2023-07-21 05:54 ` Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-21 05:54 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
On Fri, Jul 21, 2023 at 8:38 AM Michael Paquier <[email protected]> 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.
Okay. Here's a quick patch for adding TAP tests to the worker_spi
module. We can change it to taste.
Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v1-0001-Add-TAP-tests-to-worker_spi-module.patch (5.2K, ../../CALj2ACVUxgcQaGh3HpBJ6Xg9FRqj7UNDomd5E1y=tRKs2wcntA@mail.gmail.com/2-v1-0001-Add-TAP-tests-to-worker_spi-module.patch)
download | inline diff:
From 58c9f33c2e6e0a2aa6b1466be4f186e5e683afc1 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Fri, 21 Jul 2023 05:52:32 +0000
Subject: [PATCH v1] Add TAP tests to worker_spi module
---
src/test/modules/worker_spi/Makefile | 2 +
src/test/modules/worker_spi/dynamic.conf | 3 +-
src/test/modules/worker_spi/meson.build | 5 ++
.../modules/worker_spi/t/001_worker_spi.pl | 52 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 8 +--
5 files changed, 65 insertions(+), 5 deletions(-)
create mode 100644 src/test/modules/worker_spi/t/001_worker_spi.pl
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index cbf9b2e37f..3f63068514 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -14,6 +14,8 @@ REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/worker_spi/dynamic.c
# Disable installcheck to ensure we cover dynamic bgworkers.
NO_INSTALLCHECK = 1
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/worker_spi/dynamic.conf b/src/test/modules/worker_spi/dynamic.conf
index bfe015f664..71221d8a3e 100644
--- a/src/test/modules/worker_spi/dynamic.conf
+++ b/src/test/modules/worker_spi/dynamic.conf
@@ -1,2 +1,3 @@
-shared_preload_libraries = worker_spi
+# Let's disable static bg workers in SQL tests and enable them in TAP tests
+worker_spi.total_workers = 0
worker_spi.database = contrib_regression
diff --git a/src/test/modules/worker_spi/meson.build b/src/test/modules/worker_spi/meson.build
index a8cdfdeb36..29ee875690 100644
--- a/src/test/modules/worker_spi/meson.build
+++ b/src/test/modules/worker_spi/meson.build
@@ -33,4 +33,9 @@ tests += {
'regress_args': ['--temp-config', files('dynamic.conf')],
'runningcheck': false,
},
+ 'tap': {
+ 'tests': [
+ 't/001_worker_spi.pl',
+ ],
+ },
}
diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
new file mode 100644
index 0000000000..e7dc6be070
--- /dev/null
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -0,0 +1,52 @@
+# 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.
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Test set-up
+my $node = PostgreSQL::Test::Cluster->new('mynode');
+$node->init;
+$node->start;
+
+$node->safe_psql('postgres', q(CREATE DATABASE mydb;));
+
+$node->append_conf('postgresql.conf', q{
+shared_preload_libraries = 'worker_spi'
+worker_spi.database = 'mydb'
+worker_spi.total_workers = 3
+});
+$node->restart;
+
+$node->safe_psql('mydb', 'CREATE EXTENSION worker_spi;');
+
+# Verify that worker_spi static bg workers have been launched.
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT count(*) = 3 FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi static worker';],
+ 't'),
+ 'worker_spi static bg workers launched'
+) or die "Timed out while waiting for worker_spi static bg workers to be launched";
+
+# Ask worker_spi to launch dynamic bg workers
+my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(1);');
+my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(2);');
+
+# Verify that worker_spi dynamic bg workers have been launched.
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT count(*) = 2 FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi dynamic worker' AND
+ pid IN ($worker1_pid, $worker2_pid);],
+ 't'),
+ 'worker_spi dynamic bg workers launched'
+) or die "Timed out while waiting for worker_spi dynamic bg workers to be launched";
+
+$node->stop;
+
+done_testing();
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index ada0fb8ac7..5676073218 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -343,8 +343,8 @@ _PG_init(void)
*/
for (int i = 1; i <= worker_spi_total_workers; i++)
{
- 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");
worker.bgw_main_arg = Int32GetDatum(i);
RegisterBackgroundWorker(&worker);
@@ -370,8 +370,8 @@ worker_spi_launch(PG_FUNCTION_ARGS)
worker.bgw_restart_time = BGW_NEVER_RESTART;
sprintf(worker.bgw_library_name, "worker_spi");
sprintf(worker.bgw_function_name, "worker_spi_main");
- 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");
worker.bgw_main_arg = Int32GetDatum(i);
/* set bgw_notify_pid so that we can use WaitForBackgroundWorkerStartup */
worker.bgw_notify_pid = MyProcPid;
--
2.34.1
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-21 06:24 ` Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2023-07-21 06:24 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
On Fri, Jul 21, 2023 at 11:24:08AM +0530, Bharath Rupireddy wrote:
> Okay. Here's a quick patch for adding TAP tests to the worker_spi
> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
@ 2023-07-21 06:51 ` Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-21 06:51 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
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:
> > Okay. Here's a quick patch for adding TAP tests to the worker_spi
> > 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()?
I think we can keep SQL tests around as it will help demonstrate
someone quickly write their own SQL tests.
> 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.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-21 10:35 ` Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Masahiro Ikeda @ 2023-07-21 10:35 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; [email protected]; +Cc: [email protected]
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
@ 2023-07-21 16:05 ` Bharath Rupireddy <[email protected]>
2023-07-24 01:04 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-28 02:23 ` Re: Support worker_spi to execute the function dynamically. Andres Freund <[email protected]>
0 siblings, 2 replies; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-21 16:05 UTC (permalink / raw)
To: Masahiro Ikeda <[email protected]>; +Cc: [email protected]; [email protected]
On Fri, Jul 21, 2023 at 4:05 PM Masahiro Ikeda <[email protected]> wrote:
>
> > 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.
Thanks for verifying.
> I have some questions about the patch.
>
> (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);
No, let's keep it that way.
> (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.
You're right. worker_spi.total_workers = 0 in custom.conf has no
effect. without shared_preload_libraries = worker_spi. Removed that.
> (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.
Modified.
I'm attaching the v2 patch. Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v2-0001-Add-TAP-tests-to-worker_spi-module.patch (5.7K, ../../CALj2ACWR9ncAiDF73unqdJF1dmsA2R0efGXX2624X+YVxcAVWg@mail.gmail.com/2-v2-0001-Add-TAP-tests-to-worker_spi-module.patch)
download | inline diff:
From 2498565753154c8722f54aac5f321aca41293e0d Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Fri, 21 Jul 2023 15:56:13 +0000
Subject: [PATCH v2] Add TAP tests to worker_spi module
This commit adds TAP tests to check if the worker_spi can start
the right number of static bg workers. We can't use
sql/worker_spi.sql for this because the static workers need a
database which is not there when the postmaster processes the
shared_preload_libraries setting.
The added TAP tests can also help other module developers to
quickly learn how to add TAP tests.
We kept sql/worker_spi.sql even though tests from there can be
moved to the newly added TAP test file. This is because they show
how to write SQL tests for an external module.
We also changed the names of the bg workers to make it clear which
ones are static and which ones are dynamic.
---
src/test/modules/worker_spi/Makefile | 2 +
src/test/modules/worker_spi/dynamic.conf | 2 +-
src/test/modules/worker_spi/meson.build | 5 ++
.../modules/worker_spi/t/001_worker_spi.pl | 49 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 8 +--
5 files changed, 61 insertions(+), 5 deletions(-)
create mode 100644 src/test/modules/worker_spi/t/001_worker_spi.pl
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index cbf9b2e37f..3f63068514 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -14,6 +14,8 @@ REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/worker_spi/dynamic.c
# Disable installcheck to ensure we cover dynamic bgworkers.
NO_INSTALLCHECK = 1
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/worker_spi/dynamic.conf b/src/test/modules/worker_spi/dynamic.conf
index bfe015f664..6c7c7ef6aa 100644
--- a/src/test/modules/worker_spi/dynamic.conf
+++ b/src/test/modules/worker_spi/dynamic.conf
@@ -1,2 +1,2 @@
-shared_preload_libraries = worker_spi
+# Let's disable static bg workers in SQL tests and enable them in TAP tests
worker_spi.database = contrib_regression
diff --git a/src/test/modules/worker_spi/meson.build b/src/test/modules/worker_spi/meson.build
index a8cdfdeb36..29ee875690 100644
--- a/src/test/modules/worker_spi/meson.build
+++ b/src/test/modules/worker_spi/meson.build
@@ -33,4 +33,9 @@ tests += {
'regress_args': ['--temp-config', files('dynamic.conf')],
'runningcheck': false,
},
+ 'tap': {
+ 'tests': [
+ 't/001_worker_spi.pl',
+ ],
+ },
}
diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
new file mode 100644
index 0000000000..485bd64335
--- /dev/null
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -0,0 +1,49 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Test worker_spi module functionality
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Test set-up
+my $node = PostgreSQL::Test::Cluster->new('mynode');
+$node->init;
+$node->start;
+
+$node->safe_psql('postgres', q(CREATE DATABASE mydb;));
+
+$node->append_conf('postgresql.conf', q{
+shared_preload_libraries = 'worker_spi'
+worker_spi.database = 'mydb'
+worker_spi.total_workers = 3
+});
+$node->restart;
+
+$node->safe_psql('mydb', 'CREATE EXTENSION worker_spi;');
+
+# Verify that worker_spi static bg workers have been launched.
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT count(*) = 3 FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi static worker';],
+ 't'),
+ 'worker_spi static bg workers launched'
+) or die "Timed out while waiting for worker_spi static bg workers to be launched";
+
+# Ask worker_spi to launch dynamic bg workers
+my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(1);');
+my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(2);');
+
+# Verify that worker_spi dynamic bg workers have been launched.
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT count(*) = 2 FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi dynamic worker' AND
+ pid IN ($worker1_pid, $worker2_pid);],
+ 't'),
+ 'worker_spi dynamic bg workers launched'
+) or die "Timed out while waiting for worker_spi dynamic bg workers to be launched";
+
+done_testing();
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index ada0fb8ac7..5676073218 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -343,8 +343,8 @@ _PG_init(void)
*/
for (int i = 1; i <= worker_spi_total_workers; i++)
{
- 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");
worker.bgw_main_arg = Int32GetDatum(i);
RegisterBackgroundWorker(&worker);
@@ -370,8 +370,8 @@ worker_spi_launch(PG_FUNCTION_ARGS)
worker.bgw_restart_time = BGW_NEVER_RESTART;
sprintf(worker.bgw_library_name, "worker_spi");
sprintf(worker.bgw_function_name, "worker_spi_main");
- 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");
worker.bgw_main_arg = Int32GetDatum(i);
/* set bgw_notify_pid so that we can use WaitForBackgroundWorkerStartup */
worker.bgw_notify_pid = MyProcPid;
--
2.34.1
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-24 01:04 ` Masahiro Ikeda <[email protected]>
2023-07-24 03:01 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Masahiro Ikeda @ 2023-07-24 01:04 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: [email protected]; [email protected]
On 2023-07-22 01:05, Bharath Rupireddy wrote:
> On Fri, Jul 21, 2023 at 4:05 PM Masahiro Ikeda
> <[email protected]> wrote:
>> (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.
>
> You're right. worker_spi.total_workers = 0 in custom.conf has no
> 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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-24 01:04 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
@ 2023-07-24 03:01 ` Bharath Rupireddy <[email protected]>
2023-07-24 07:26 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-24 03:01 UTC (permalink / raw)
To: Masahiro Ikeda <[email protected]>; +Cc: [email protected]; [email protected]
On Mon, Jul 24, 2023 at 6:34 AM Masahiro Ikeda <[email protected]> wrote:
>
> OK. If so, we need to remove the following comment in Makefile.
>
> > # enable our module in shared_preload_libraries for dynamic bgworkers
Done.
> I also confirmed that the tap tests work with meson and make.
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.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v3-0001-Add-TAP-tests-to-worker_spi-module.patch (6.4K, ../../CALj2ACUyBtj1ua7+pVHQ-UoyqvxMOkDrjLuYCwmAKH7c+uw6qg@mail.gmail.com/2-v3-0001-Add-TAP-tests-to-worker_spi-module.patch)
download | inline diff:
From 8c39710902e92a196a2233c1d0b9bb35dcdd5c83 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Mon, 24 Jul 2023 02:49:29 +0000
Subject: [PATCH v3] Add TAP tests to worker_spi module
This commit adds TAP tests to check if worker_spi starts
background workers as expected. sql/worker_spi.sql can't be used
for static bg workers, because the static workers need a database
which is not there when postmaster processes
shared_preload_libraries GUC setting.
The existing tests in sql/worker_spi.sql are kept as-is even though
all of them can be moved to the newly added TAP test file. These
SQL tests along with added TAP tests can help as an example for
module developers.
While here, this commit changes the names of bg workers to
distinguish static and dynamic bg workers.
---
src/test/modules/worker_spi/Makefile | 5 +-
src/test/modules/worker_spi/dynamic.conf | 1 -
src/test/modules/worker_spi/meson.build | 5 ++
.../modules/worker_spi/t/001_worker_spi.pl | 49 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 11 +++--
5 files changed, 64 insertions(+), 7 deletions(-)
create mode 100644 src/test/modules/worker_spi/t/001_worker_spi.pl
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index cbf9b2e37f..09842dc8cc 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -8,12 +8,15 @@ PGFILEDESC = "worker_spi - background worker example"
REGRESS = worker_spi
-# enable our module in shared_preload_libraries for dynamic bgworkers
+# Specify a database to connect to for dynamic bgworkers in custom
+# configuration file for SQL tests.
REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/worker_spi/dynamic.conf
# Disable installcheck to ensure we cover dynamic bgworkers.
NO_INSTALLCHECK = 1
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/worker_spi/dynamic.conf b/src/test/modules/worker_spi/dynamic.conf
index bfe015f664..78ede3d079 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
diff --git a/src/test/modules/worker_spi/meson.build b/src/test/modules/worker_spi/meson.build
index a8cdfdeb36..29ee875690 100644
--- a/src/test/modules/worker_spi/meson.build
+++ b/src/test/modules/worker_spi/meson.build
@@ -33,4 +33,9 @@ tests += {
'regress_args': ['--temp-config', files('dynamic.conf')],
'runningcheck': false,
},
+ 'tap': {
+ 'tests': [
+ 't/001_worker_spi.pl',
+ ],
+ },
}
diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
new file mode 100644
index 0000000000..485bd64335
--- /dev/null
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -0,0 +1,49 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Test worker_spi module functionality
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Test set-up
+my $node = PostgreSQL::Test::Cluster->new('mynode');
+$node->init;
+$node->start;
+
+$node->safe_psql('postgres', q(CREATE DATABASE mydb;));
+
+$node->append_conf('postgresql.conf', q{
+shared_preload_libraries = 'worker_spi'
+worker_spi.database = 'mydb'
+worker_spi.total_workers = 3
+});
+$node->restart;
+
+$node->safe_psql('mydb', 'CREATE EXTENSION worker_spi;');
+
+# Verify that worker_spi static bg workers have been launched.
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT count(*) = 3 FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi static worker';],
+ 't'),
+ 'worker_spi static bg workers launched'
+) or die "Timed out while waiting for worker_spi static bg workers to be launched";
+
+# Ask worker_spi to launch dynamic bg workers
+my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(1);');
+my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(2);');
+
+# Verify that worker_spi dynamic bg workers have been launched.
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT count(*) = 2 FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi dynamic worker' AND
+ pid IN ($worker1_pid, $worker2_pid);],
+ 't'),
+ 'worker_spi dynamic bg workers launched'
+) or die "Timed out while waiting for worker_spi dynamic bg workers to be launched";
+
+done_testing();
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index ada0fb8ac7..461f272e77 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -5,7 +5,8 @@
* patterns: establishing a database connection; starting and committing
* transactions; using GUC variables, and heeding SIGHUP to reread
* the configuration file; reporting to pg_stat_activity; using the
- * process latch to sleep and exit in case of postmaster death.
+ * process latch to sleep and exit in case of postmaster death; writing
+ * regression tests, both core (sql) and extended (TAP) tests.
*
* This code connects to a database, creates a schema and table, and summarizes
* the numbers contained therein. To see it working, insert an initial value
@@ -343,8 +344,8 @@ _PG_init(void)
*/
for (int i = 1; i <= worker_spi_total_workers; i++)
{
- 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");
worker.bgw_main_arg = Int32GetDatum(i);
RegisterBackgroundWorker(&worker);
@@ -370,8 +371,8 @@ worker_spi_launch(PG_FUNCTION_ARGS)
worker.bgw_restart_time = BGW_NEVER_RESTART;
sprintf(worker.bgw_library_name, "worker_spi");
sprintf(worker.bgw_function_name, "worker_spi_main");
- 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");
worker.bgw_main_arg = Int32GetDatum(i);
/* set bgw_notify_pid so that we can use WaitForBackgroundWorkerStartup */
worker.bgw_notify_pid = MyProcPid;
--
2.34.1
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-24 01:04 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-24 03:01 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-24 07:26 ` Masahiro Ikeda <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Masahiro Ikeda @ 2023-07-24 07:26 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: [email protected]; [email protected]
On 2023-07-24 12:01, Bharath Rupireddy wrote:
> 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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
@ 2023-07-28 02:23 ` Andres Freund <[email protected]>
2023-07-28 04:45 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Andres Freund @ 2023-07-28 02:23 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]; [email protected]
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
Attachments:
[text/x-diff] repro.diff (1.4K, ../../[email protected]/2-repro.diff)
download | inline diff:
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-28 02:23 ` Re: Support worker_spi to execute the function dynamically. Andres Freund <[email protected]>
@ 2023-07-28 04:45 ` Michael Paquier <[email protected]>
2023-07-28 05:17 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2023-07-28 04:45 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Masahiro Ikeda <[email protected]>; [email protected]
On Thu, Jul 27, 2023 at 07:23:32PM -0700, Andres Freund wrote:
> 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...
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 protected]
--
Michael
Attachments:
[text/x-diff] worker_spi-race.patch (1.8K, ../../[email protected]/2-worker_spi-race.patch)
download | inline diff:
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";
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-28 02:23 ` Re: Support worker_spi to execute the function dynamically. Andres Freund <[email protected]>
2023-07-28 04:45 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
@ 2023-07-28 05:17 ` Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Bharath Rupireddy @ 2023-07-28 05:17 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Masahiro Ikeda <[email protected]>; [email protected]
On Fri, Jul 28, 2023 at 10:15 AM Michael Paquier <[email protected]> wrote:
>
> 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?
+# check their existence. Use IDs that do not overlap with the schemas created
+# by the previous workers.
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.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2023-07-28 05:17 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-10-11 01:03 [PATCH 2/7] Write WAL for empty nbtree index build Kyotaro Horiguchi <[email protected]>
2023-07-20 04:13 Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 04:39 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 04:50 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-20 09:08 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 10:14 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 03:08 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 05:54 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 06:24 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-21 06:51 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-21 10:35 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-21 16:05 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-24 01:04 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-24 03:01 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
2023-07-24 07:26 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-28 02:23 ` Re: Support worker_spi to execute the function dynamically. Andres Freund <[email protected]>
2023-07-28 04:45 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-28 05:17 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox