Re: coverage increase for worker_spi
On 2019-Jun-01, Tom Lane wrote: > Alvaro Herrera writes: > > I ended up with these two patches. I'm not sure about pushing > > separately. It seems pointless to backport the "fix" to back branches > > anyway. > > Patch passes the eyeball test, though I did not try to run it. > I concur with squashing into one commit and applying to HEAD only. Okay, pushed. Let's see how it does, now. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: coverage increase for worker_spi
Alvaro Herrera writes: > I ended up with these two patches. I'm not sure about pushing > separately. It seems pointless to backport the "fix" to back branches > anyway. Patch passes the eyeball test, though I did not try to run it. I concur with squashing into one commit and applying to HEAD only. regards, tom lane
Re: coverage increase for worker_spi
On 2019-May-30, Alvaro Herrera wrote:
> One thing I noticed while writing it, though, is that worker_spi uses
> the postgres database, instead of the contrib_regression database that
> was created for it. And we create a schema and a table there. This is
> going to get some eyebrows raised, I think, so I'll look into fixing
> that as a bugfix before getting this commit in.
Another thing I noticed when fixing *this*, in turn, is that if you load
worker_spi in shared_preload_libraries then the contrib_regression
database doesn't exist by the point that runs, so those workers fail to
start. The dynamic one does start in the configured database.
I guess we could just ignore the failures and just rely on the dynamic
worker.
I ended up with these two patches. I'm not sure about pushing
separately. It seems pointless to backport the "fix" to back branches
anyway.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index c1878dd694..bc9ef64a50 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -55,6 +55,7 @@ static volatile sig_atomic_t got_sigterm = false;
/* GUC variables */
static int worker_spi_naptime = 10;
static int worker_spi_total_workers = 2;
+static char *worker_spi_database = NULL;
typedef struct worktable
@@ -179,7 +180,7 @@ worker_spi_main(Datum main_arg)
BackgroundWorkerUnblockSignals();
/* Connect to our database */
- BackgroundWorkerInitializeConnection("postgres", NULL, 0);
+ BackgroundWorkerInitializeConnection(worker_spi_database, NULL, 0);
elog(LOG, "%s initialized with %s.%s",
MyBgworkerEntry->bgw_name, table->schema, table->name);
@@ -339,6 +340,15 @@ _PG_init(void)
NULL,
NULL);
+ DefineCustomStringVariable("worker_spi.database",
+ "Database to connect to.",
+ NULL,
+ _spi_database,
+ "postgres",
+ PGC_POSTMASTER,
+ 0,
+ NULL, NULL, NULL);
+
/* set up common data for all our workers */
memset(, 0, sizeof(worker));
worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
--
2.17.1
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index 7cdb33c9df..cbf9b2e37f 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -6,6 +6,14 @@ 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
+
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
new file mode 100644
index 00..bfe015f664
--- /dev/null
+++ b/src/test/modules/worker_spi/dynamic.conf
@@ -0,0 +1,2 @@
+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
new file mode 100644
index 00..dc0a79bf75
--- /dev/null
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -0,0 +1,50 @@
+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/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
new file mode 100644
index 00..4683523b29
--- /dev/null
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -0,0 +1,35 @@
+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 :=
Re: coverage increase for worker_spi
Alvaro Herrera writes: > On 2019-May-30, Tom Lane wrote: >> Hm, I don't understand how this works at all: >> >> +PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 >> END) >> +FROM schema1.counted WHERE type = 'delta'; >> +GET DIAGNOSTICS count = ROW_COUNT; >> >> Given that it uses an aggregate, the ROW_COUNT must always be 1, no? > Well, I was surprised to see the count(*) work there as an argument for > pg_sleep there at all frankly (maybe we are sleeping 0.1s more than we > really need, per your observation), but the row_count is concerned with > rows that have type = 'delta', which are deleted by the bgworker. So > the test script job is done when the bgworker has run once through its > loop. No, the row_count is going to report the number of rows returned by the aggregate query, which is going to be one row, independently of how many rows went into the aggregate. regression=# do $$ declare c int; begin perform count(*) from tenk1; get diagnostics c = row_count; raise notice 'c = %', c; end$$; psql: NOTICE: c = 1 DO regression=# do $$ declare c int; begin perform count(*) from tenk1 where false; get diagnostics c = row_count; raise notice 'c = %', c; end$$; psql: NOTICE: c = 1 DO I think you want to capture the actual aggregate output rather than relying on row_count: regression=# do $$ declare c int; begin c := count(*) from tenk1; raise notice 'c = %', c; end$$; psql: NOTICE: c = 1 DO regression=# do $$ declare c int; begin c := count(*) from tenk1 where false; raise notice 'c = %', c; end$$; psql: NOTICE: c = 0 DO regards, tom lane
Re: coverage increase for worker_spi
On 2019-May-30, Tom Lane wrote: > Alvaro Herrera writes: > > On 2019-May-29, Tom Lane wrote: > >> I'm not opposed to adding a new test case at this point in the cycle, > >> but as written this one seems more or less guaranteed to fail under > >> load. > > > True. Here's a version that should be more resilient. > > Hm, I don't understand how this works at all: > > + PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 > END) > + FROM schema1.counted WHERE type = 'delta'; > + GET DIAGNOSTICS count = ROW_COUNT; > > Given that it uses an aggregate, the ROW_COUNT must always be 1, no? Well, I was surprised to see the count(*) work there as an argument for pg_sleep there at all frankly (maybe we are sleeping 0.1s more than we really need, per your observation), but the row_count is concerned with rows that have type = 'delta', which are deleted by the bgworker. So the test script job is done when the bgworker has run once through its loop. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: coverage increase for worker_spi
Alvaro Herrera writes: > On 2019-May-29, Tom Lane wrote: >> I'm not opposed to adding a new test case at this point in the cycle, >> but as written this one seems more or less guaranteed to fail under >> load. > True. Here's a version that should be more resilient. Hm, I don't understand how this works at all: + PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 END) + FROM schema1.counted WHERE type = 'delta'; + GET DIAGNOSTICS count = ROW_COUNT; Given that it uses an aggregate, the ROW_COUNT must always be 1, no? regards, tom lane
Re: coverage increase for worker_spi
On 2019-May-29, Tom Lane wrote:
> Alvaro Herrera writes:
> > Tom pointed out that coverage for worker_spi is 0%. For a module that
> > only exists to provide coverage, that's pretty stupid. This patch
> > increases coverage to 90.9% line-wise and 100% function-wise, which
> > seems like a sufficient starting point.
>
> > How would people feel about me getting this in master at this point in
> > the cycle, it being just some test code? We can easily revert if
> > it seems too unstable.
>
> I'm not opposed to adding a new test case at this point in the cycle,
> but as written this one seems more or less guaranteed to fail under
> load.
True. Here's a version that should be more resilient.
One thing I noticed while writing it, though, is that worker_spi uses
the postgres database, instead of the contrib_regression database that
was created for it. And we create a schema and a table there. This is
going to get some eyebrows raised, I think, so I'll look into fixing
that as a bugfix before getting this commit in.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index 7cdb33c9df..cbf9b2e37f 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -6,6 +6,14 @@ 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
+
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
new file mode 100644
index 00..646885a9c7
--- /dev/null
+++ b/src/test/modules/worker_spi/dynamic.conf
@@ -0,0 +1,2 @@
+worker_spi.naptime = 1
+shared_preload_libraries = worker_spi
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
new file mode 100644
index 00..cf1f252e29
--- /dev/null
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -0,0 +1,37 @@
+\c postgres
+CREATE EXTENSION worker_spi;
+SELECT worker_spi_launch(1) IS NOT NULL;
+ ?column?
+--
+ t
+(1 row)
+
+INSERT INTO schema1.counted VALUES ('total', 0), ('delta', 1);
+SELECT pg_reload_conf();
+ pg_reload_conf
+
+ t
+(1 row)
+
+DO $$
+ DECLARE
+ count int;
+ loops int := 0;
+ BEGIN
+ LOOP
+ PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 END)
+ FROM schema1.counted WHERE type = 'delta';
+ GET DIAGNOSTICS count = ROW_COUNT;
+ loops := loops + 1;
+ IF count < 1 OR loops > 180 * 10 THEN
+RETURN;
+ END IF;
+ END LOOP;
+END
+$$;
+SELECT * FROM schema1.counted;
+ type | value
+---+---
+ total | 1
+(1 row)
+
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
new file mode 100644
index 00..bae2680dfb
--- /dev/null
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -0,0 +1,22 @@
+\c postgres
+CREATE EXTENSION worker_spi;
+SELECT worker_spi_launch(1) IS NOT NULL;
+INSERT INTO schema1.counted VALUES ('total', 0), ('delta', 1);
+SELECT pg_reload_conf();
+DO $$
+ DECLARE
+ count int;
+ loops int := 0;
+ BEGIN
+ LOOP
+ PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 END)
+ FROM schema1.counted WHERE type = 'delta';
+ GET DIAGNOSTICS count = ROW_COUNT;
+ loops := loops + 1;
+ IF count < 1 OR loops > 180 * 10 THEN
+RETURN;
+ END IF;
+ END LOOP;
+END
+$$;
+SELECT * FROM schema1.counted;
--
2.17.1
Re: coverage increase for worker_spi
Alvaro Herrera writes: > Tom pointed out that coverage for worker_spi is 0%. For a module that > only exists to provide coverage, that's pretty stupid. This patch > increases coverage to 90.9% line-wise and 100% function-wise, which > seems like a sufficient starting point. > How would people feel about me getting this in master at this point in > the cycle, it being just some test code? We can easily revert if > it seems too unstable. I'm not opposed to adding a new test case at this point in the cycle, but as written this one seems more or less guaranteed to fail under load. You can't just sleep for worker_spi.naptime and expect that the worker will certainly have run. Perhaps you could use a plpgsql DO block with a loop to wait up to X seconds until the expected state appears, for X around 120 to 180 seconds (compare poll_query_until in the TAP tests). regards, tom lane
coverage increase for worker_spi
Tom pointed out that coverage for worker_spi is 0%. For a module that
only exists to provide coverage, that's pretty stupid. This patch
increases coverage to 90.9% line-wise and 100% function-wise, which
seems like a sufficient starting point.
How would people feel about me getting this in master at this point in
the cycle, it being just some test code? We can easily revert if
it seems too unstable.
--
Álvaro Herrera
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index 7cdb33c9df..cbf9b2e37f 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -6,6 +6,14 @@ 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
+
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
new file mode 100644
index 00..646885a9c7
--- /dev/null
+++ b/src/test/modules/worker_spi/dynamic.conf
@@ -0,0 +1,2 @@
+worker_spi.naptime = 1
+shared_preload_libraries = worker_spi
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
new file mode 100644
index 00..28b2970d01
--- /dev/null
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -0,0 +1,22 @@
+\c postgres
+CREATE EXTENSION worker_spi;
+SELECT worker_spi_launch(1) IS NOT NULL;
+ ?column?
+--
+ t
+(1 row)
+
+INSERT INTO schema1.counted VALUES ('total', 0), ('delta', 1);
+SELECT pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE current_setting('worker_spi.naptime')::int END)
+ FROM schema1.counted WHERE type = 'delta';
+ pg_sleep
+--
+
+(1 row)
+
+SELECT * FROM schema1.counted;
+ type | value
+---+---
+ total | 1
+(1 row)
+
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
new file mode 100644
index 00..ac454cff57
--- /dev/null
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -0,0 +1,7 @@
+\c postgres
+CREATE EXTENSION worker_spi;
+SELECT worker_spi_launch(1) IS NOT NULL;
+INSERT INTO schema1.counted VALUES ('total', 0), ('delta', 1);
+SELECT pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE current_setting('worker_spi.naptime')::int END)
+ FROM schema1.counted WHERE type = 'delta';
+SELECT * FROM schema1.counted;
--
2.17.1
