public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bharath Rupireddy <[email protected]>
To: Masahiro Ikeda <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: Support worker_spi to execute the function dynamically.
Date: Fri, 21 Jul 2023 21:35:06 +0530
Message-ID: <CALj2ACWR9ncAiDF73unqdJF1dmsA2R0efGXX2624X+YVxcAVWg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<ZLiwP5vrT/[email protected]>
<CALj2ACVOCiXirZFRL-gi7mV2zgdH4e3-vyJQx2fxdzycJTv3qg@mail.gmail.com>
<[email protected]>
<CALj2ACWj7qF-YYVi05okBFJnPugkwsM8PSrEwfHs-tQqKO80Uw@mail.gmail.com>
<[email protected]>
<CALj2ACV0fr-9OTZqtWu-9XrtUbBZCYb0UK859gfvJ0oqm3YvUA@mail.gmail.com>
<ZLn2v/[email protected]>
<CALj2ACVUxgcQaGh3HpBJ6Xg9FRqj7UNDomd5E1y=tRKs2wcntA@mail.gmail.com>
<[email protected]>
<CALj2ACVWW2NU78kJ=xEbz8qcZgWeae+cVp8TYbvGTdpYjVKjjA@mail.gmail.com>
<[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
view thread (18+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Support worker_spi to execute the function dynamically.
In-Reply-To: <CALj2ACWR9ncAiDF73unqdJF1dmsA2R0efGXX2624X+YVxcAVWg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox