On Mon, Jul 24, 2023 at 08:31:01AM +0530, Bharath Rupireddy wrote: > I also added a note atop worker_spi.c that the module also > demonstrates how to write core (SQL) tests and extended (TAP) tests.
The value of the SQL tests comes down to the DO blocks that emulate what the TAP tests could equally be able to do. While we already have some places that do something similar (slot.sql or postgres_fdw.sql), the SQL tests of worker_spi count for a total of five queries, which is not much with one cluster initialized: - One pg_reload_conf() to work a loop to happen in the worker. - Two sanity checks. - Two wait emulations. Anyway, most people that do serious hacking on this list care about the runtime of the tests all the time, and I am not on board in making things slower for the sake of showing a test example here particularly if there are ways to make them faster (long-term, we should be able to do the init step only once for most cases), and because we *have to* switch to TAP to have more advanced scenarios for the custom wait events or just dynamic work launches based on what we set on shared_preload_libraries. On top of that, we have other examples in the tree that emulate waits for plain SQL tests to satisfy assumptions with some follow-up query. So, I don't really agree with the value gained here compared to the execution cost of initializing two clusters for this module. I have taken the time to check how the runtime changes when switching to TAP for all the scenarios discussed here, and from my laptop, I can see that: - HEAD takes 4.4s, for only the sql/ test. - Your latest patch is at 5.6s. - My version attached to this message is at 3.7s. In terms of runtime the benefits are here for me. Note that with the first part of the test (previously in sql/), we don't lose coverage with the loop of the workers so I agree that only checking that these are launched is OK once worker_spi is in shared_preload_libraries. However, I think that we should make sure that they are connected to the correct database 'mydb'. I have updated the test to do that. So, what do you think about the attached? -- Michael
From ea9f1b905723716037d9f44b9afbcd05075e9b27 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 24 Jul 2023 16:37:10 +0900
Subject: [PATCH v4] 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 is gone, replaced by an
equivalent in TAP.
While here, this commit changes the names of bg workers to
distinguish static and dynamic bg workers.
---
src/test/modules/worker_spi/Makefile | 8 +-
src/test/modules/worker_spi/dynamic.conf | 2 -
.../worker_spi/expected/worker_spi.out | 50 -------------
src/test/modules/worker_spi/meson.build | 9 +--
.../modules/worker_spi/sql/worker_spi.sql | 35 ---------
.../modules/worker_spi/t/001_worker_spi.pl | 75 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 8 +-
7 files changed, 83 insertions(+), 104 deletions(-)
delete mode 100644 src/test/modules/worker_spi/dynamic.conf
delete mode 100644 src/test/modules/worker_spi/expected/worker_spi.out
delete mode 100644 src/test/modules/worker_spi/sql/worker_spi.sql
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..024b34cdbb 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -6,13 +6,7 @@ EXTENSION = worker_spi
DATA = worker_spi--1.0.sql
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.
-NO_INSTALLCHECK = 1
+TAP_TESTS = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/test/modules/worker_spi/dynamic.conf b/src/test/modules/worker_spi/dynamic.conf
deleted file mode 100644
index bfe015f664..0000000000
--- a/src/test/modules/worker_spi/dynamic.conf
+++ /dev/null
@@ -1,2 +0,0 @@
-shared_preload_libraries = worker_spi
-worker_spi.database = contrib_regression
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
deleted file mode 100644
index dc0a79bf75..0000000000
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ /dev/null
@@ -1,50 +0,0 @@
-CREATE EXTENSION worker_spi;
-SELECT worker_spi_launch(4) IS NOT NULL;
- ?column?
-----------
- t
-(1 row)
-
--- wait until the worker completes its initialization
-DO $$
-DECLARE
- visible bool;
- loops int := 0;
-BEGIN
- LOOP
- visible := table_name IS NOT NULL
- FROM information_schema.tables
- WHERE table_schema = 'schema4' AND table_name = 'counted';
- IF visible OR loops > 120 * 10 THEN EXIT; END IF;
- PERFORM pg_sleep(0.1);
- loops := loops + 1;
- END LOOP;
-END
-$$;
-INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
-SELECT pg_reload_conf();
- pg_reload_conf
-----------------
- t
-(1 row)
-
--- wait until the worker has processed the tuple we just inserted
-DO $$
-DECLARE
- count int;
- loops int := 0;
-BEGIN
- LOOP
- count := count(*) FROM schema4.counted WHERE type = 'delta';
- IF count = 0 OR loops > 120 * 10 THEN EXIT; END IF;
- PERFORM pg_sleep(0.1);
- loops := loops + 1;
- END LOOP;
-END
-$$;
-SELECT * FROM schema4.counted;
- type | value
--------+-------
- total | 1
-(1 row)
-
diff --git a/src/test/modules/worker_spi/meson.build b/src/test/modules/worker_spi/meson.build
index a8cdfdeb36..68870324c9 100644
--- a/src/test/modules/worker_spi/meson.build
+++ b/src/test/modules/worker_spi/meson.build
@@ -25,12 +25,9 @@ tests += {
'name': 'worker_spi',
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
- 'regress': {
- 'sql': [
- 'worker_spi',
+ 'tap': {
+ 'tests': [
+ 't/001_worker_spi.pl',
],
- 'dbname': 'contrib_regression',
- 'regress_args': ['--temp-config', files('dynamic.conf')],
- 'runningcheck': false,
},
}
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
deleted file mode 100644
index 4683523b29..0000000000
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ /dev/null
@@ -1,35 +0,0 @@
-CREATE EXTENSION worker_spi;
-SELECT worker_spi_launch(4) IS NOT NULL;
--- wait until the worker completes its initialization
-DO $$
-DECLARE
- visible bool;
- loops int := 0;
-BEGIN
- LOOP
- visible := table_name IS NOT NULL
- FROM information_schema.tables
- WHERE table_schema = 'schema4' AND table_name = 'counted';
- IF visible OR loops > 120 * 10 THEN EXIT; END IF;
- PERFORM pg_sleep(0.1);
- loops := loops + 1;
- END LOOP;
-END
-$$;
-INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
-SELECT pg_reload_conf();
--- wait until the worker has processed the tuple we just inserted
-DO $$
-DECLARE
- count int;
- loops int := 0;
-BEGIN
- LOOP
- count := count(*) FROM schema4.counted WHERE type = 'delta';
- IF count = 0 OR loops > 120 * 10 THEN EXIT; END IF;
- PERFORM pg_sleep(0.1);
- loops := loops + 1;
- END LOOP;
-END
-$$;
-SELECT * FROM schema4.counted;
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..4f1943750e
--- /dev/null
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -0,0 +1,75 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Test worker_spi module.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('mynode');
+$node->init;
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION worker_spi;');
+
+# Launch one dynamic worker, then wait for its initialization to complete.
+# This consists in making sure that a table name "counted" is created
+# on a new schema whose name includes the index defined in input argument
+# of worker_spi_launch().
+# By default, dynamic bgworkers connect to the "postgres" database.
+$node->safe_psql('postgres', 'SELECT worker_spi_launch(4) IS NOT NULL;');
+$node->poll_query_until(
+ 'postgres',
+ qq[SELECT count(*) > 0 FROM information_schema.tables
+ WHERE table_schema = 'schema4' AND table_name = 'counted';]);
+$node->safe_psql('postgres',
+ "INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);");
+# Issue a SIGHUP on the node to force the worker to loop once, accelerating
+# this test.
+$node->reload;
+# Wait until the worker has processed the tuple we just inserted.
+$node->poll_query_until('postgres',
+ qq[SELECT count(*) FROM schema4.counted WHERE type = 'delta';], '0');
+my $result = $node->safe_psql('postgres', 'SELECT * FROM schema4.counted;');
+is($result, qq(total|1), 'dynamic worker correctly consumed tuple data');
+
+# Create the database first so as the workers can connect to it when
+# the library is loaded.
+$node->safe_psql('postgres', q(CREATE DATABASE mydb;));
+$node->safe_psql('mydb', 'CREATE EXTENSION worker_spi;');
+
+# Now load the module as a shared library.
+$node->append_conf(
+ 'postgresql.conf', q{
+shared_preload_libraries = 'worker_spi'
+worker_spi.database = 'mydb'
+worker_spi.total_workers = 3
+});
+$node->restart;
+
+# Check that static bgworkers have been launched.
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT datname, count(datname) FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi static worker' GROUP BY datname;],
+ 'mydb|3'),
+ 'static bgworkers all launched')
+ or die
+ "Timed out while waiting for static bgworkers to be launched";
+
+# Ask worker_spi to launch dynamic bgworkers, and check their presence.
+my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(1);');
+my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(2);');
+ok( $node->poll_query_until(
+ 'mydb',
+ qq[SELECT datname, count(datname) FROM pg_stat_activity
+ WHERE backend_type = 'worker_spi dynamic worker' AND
+ pid IN ($worker1_pid, $worker2_pid) GROUP BY datname;],
+ 'mydb|2'),
+ 'dynamic bgworkers all launched')
+ or die
+ "Timed out while waiting for dynamic bgworkers 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.40.1
signature.asc
Description: PGP signature
