pgsql: Disable installcheck tests for test_session_hooks

Started by Andrew Dunstanabout 8 years ago7 messages

andrew@dunslane.net

about 8 years ago

Disable installcheck tests for test_session_hooks

The module requires a preloaded library and the defect can't be cured by
a LOAD instruction in the test script. To achieve this we override the
installcheck target in the module's Makefile, and exclude ithe module in
vcregress.pl.

Along the way, revert commit 9989f92aabd.

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/745948422c799c1b9f976ee30f21a7aac050e0f3

Modified Files
--------------
src/test/modules/Makefile | 1 +
src/test/modules/test_session_hooks/Makefile | 4 ++++
src/tools/msvc/vcregress.pl | 2 ++
3 files changed, 7 insertions(+)

Tom Lane

tgl@sss.pgh.pa.us

about 8 years ago

In reply to: Andrew Dunstan (#1)

Re: pgsql: Disable installcheck tests for test_session_hooks

Andrew Dunstan <andrew@dunslane.net> writes:

Disable installcheck tests for test_session_hooks

This seems not quite right; it's causing warnings like this:

$ make -s clean
Makefile:25: warning: overriding commands for target `installcheck'
../../../../src/makefiles/pgxs.mk:275: warning: ignoring old commands for target `installcheck'

regards, tom lane

Michael Paquier

michael.paquier@gmail.com

about 8 years ago

In reply to: Tom Lane (#2)

1 attachment(s)

Re: pgsql: Disable installcheck tests for test_session_hooks

On Thu, Nov 16, 2017 at 11:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Disable installcheck tests for test_session_hooks

This seems not quite right; it's causing warnings like this:

$ make -s clean
Makefile:25: warning: overriding commands for target `installcheck'
../../../../src/makefiles/pgxs.mk:275: warning: ignoring old commands for target `installcheck'

This is caused by the fact that installcheck gets defined because of
REGRESS. This stuff should do something similar to what is done in
contrib/test_decoding/Makefile, and I think that it would be useful to
have the option to force installcheck on an installation that has all
the needed parameters set. With this last point, I think that the
test_session_hooks.sql should drop the roles it created. Attached is a
cleanup patch (includes fixes for compilation warnings).
--
Michael

Attachments:

session_hook_makefile_clean.patchapplication/octet-stream; name=session_hook_makefile_clean.patchDownload

diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
index 636ae61c0e..906aee0aec 100644
--- a/src/test/modules/test_session_hooks/Makefile
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -6,7 +6,6 @@ PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
 EXTENSION = test_session_hooks
 DATA = test_session_hooks--1.0.sql
 
-REGRESS = test_session_hooks
 REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
 
 ifdef USE_PGXS
@@ -20,6 +19,32 @@ include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif
 
-# override installcheck - this module requires preloading the test module
-installcheck:
-	@echo Cannot run $@ for test_session_hooks. Run "'make check'" instead.
+# Disabled because these tests require shared_preload_libraries to be
+# set but users unlikely have that set up.
+installcheck:;
+
+# But it can nonetheless be very helpful to run tests on preexisting
+# installation, allow to do so, but only if requested explicitly.
+installcheck-force: regresscheck-install-force
+
+check: regresscheck
+
+submake-regress:
+	$(MAKE) -C $(top_builddir)/src/test/regress all
+
+submake-test_session_hooks:
+	$(MAKE) -C $(top_builddir)/src/test/modules/test_session_hooks
+
+REGRESSCHECKS = test_session_hooks
+
+regresscheck: | submake-regress submake-test_session_hooks temp-install
+	$(pg_regress_check) $(REGRESS_OPTS) $(REGRESSCHECKS)
+
+regresscheck-install-force: | submake-regress submake-test_decoding temp-install
+	$(pg_regress_installcheck) \
+	    $(REGRESSCHECKS)
+
+.PHONY: submake-test_decoding submake-regress check \
+	regresscheck regresscheck-install-force
+
+temp-install: EXTRA_INSTALL=src/test/modules/test_session_hooks
diff --git a/src/test/modules/test_session_hooks/expected/test_session_hooks.out b/src/test/modules/test_session_hooks/expected/test_session_hooks.out
index be1b94953c..8c1a92f7e8 100644
--- a/src/test/modules/test_session_hooks/expected/test_session_hooks.out
+++ b/src/test/modules/test_session_hooks/expected/test_session_hooks.out
@@ -16,16 +16,18 @@ SELECT * FROM session_hook_log ORDER BY id;
 
 \c :prevdb regress_sess_hook_usr2
 SELECT * FROM session_hook_log ORDER BY id;
- id |       dbname       |        username        | hook_at 
-----+--------------------+------------------------+---------
-  1 | contrib_regression | regress_sess_hook_usr2 | START
+ id |   dbname   |        username        | hook_at 
+----+------------+------------------------+---------
+  1 | regression | regress_sess_hook_usr2 | START
 (1 row)
 
 \c :prevdb :prevusr
 SELECT * FROM session_hook_log ORDER BY id;
- id |       dbname       |        username        | hook_at 
-----+--------------------+------------------------+---------
-  1 | contrib_regression | regress_sess_hook_usr2 | START
-  2 | contrib_regression | regress_sess_hook_usr2 | END
+ id |   dbname   |        username        | hook_at 
+----+------------+------------------------+---------
+  1 | regression | regress_sess_hook_usr2 | START
+  2 | regression | regress_sess_hook_usr2 | END
 (2 rows)
 
+DROP ROLE regress_sess_hook_usr1;
+DROP ROLE regress_sess_hook_usr2;
diff --git a/src/test/modules/test_session_hooks/sql/test_session_hooks.sql b/src/test/modules/test_session_hooks/sql/test_session_hooks.sql
index 5e0864753d..9644c5a694 100644
--- a/src/test/modules/test_session_hooks/sql/test_session_hooks.sql
+++ b/src/test/modules/test_session_hooks/sql/test_session_hooks.sql
@@ -10,3 +10,5 @@ SELECT * FROM session_hook_log ORDER BY id;
 SELECT * FROM session_hook_log ORDER BY id;
 \c :prevdb :prevusr
 SELECT * FROM session_hook_log ORDER BY id;
+DROP ROLE regress_sess_hook_usr1;
+DROP ROLE regress_sess_hook_usr2;
diff --git a/src/test/modules/test_session_hooks/test_session_hooks.c b/src/test/modules/test_session_hooks/test_session_hooks.c
index 4e2eef183e..04acd27eeb 100644
--- a/src/test/modules/test_session_hooks/test_session_hooks.c
+++ b/src/test/modules/test_session_hooks/test_session_hooks.c
@@ -72,7 +72,7 @@ register_session_hook(const char *hook_at)
 
 /* sample session start hook function */
 static void
-sample_session_start_hook()
+sample_session_start_hook(void)
 {
 	/* Hook just normal backends */
 	if (MyBackendId != InvalidBackendId)
@@ -86,7 +86,7 @@ sample_session_start_hook()
 
 /* sample session end hook function */
 static void
-sample_session_end_hook()
+sample_session_end_hook(void)
 {
 	/* Hook just normal backends */
 	if (MyBackendId != InvalidBackendId)

Michael Paquier

michael.paquier@gmail.com

about 8 years ago

In reply to: Michael Paquier (#3)

1 attachment(s)

Re: pgsql: Disable installcheck tests for test_session_hooks

On Thu, Nov 16, 2017 at 3:45 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Nov 16, 2017 at 11:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Disable installcheck tests for test_session_hooks

This seems not quite right; it's causing warnings like this:

$ make -s clean
Makefile:25: warning: overriding commands for target `installcheck'
../../../../src/makefiles/pgxs.mk:275: warning: ignoring old commands for target `installcheck'

This is caused by the fact that installcheck gets defined because of
REGRESS. This stuff should do something similar to what is done in
contrib/test_decoding/Makefile, and I think that it would be useful to
have the option to force installcheck on an installation that has all
the needed parameters set. With this last point, I think that the
test_session_hooks.sql should drop the roles it created. Attached is a
cleanup patch (includes fixes for compilation warnings).

Just sending an updated patch to cope with the fact that this module
does not need to define the EXTENSION and DATA fields, and does not
need a sql and a control file..
--
Michael

Attachments:

session_hook_makefile_clean_v2.patchapplication/octet-stream; name=session_hook_makefile_clean_v2.patchDownload

diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
index 636ae61c0e..6a33eee396 100644
--- a/src/test/modules/test_session_hooks/Makefile
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -3,10 +3,6 @@
 MODULES = test_session_hooks
 PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
 
-EXTENSION = test_session_hooks
-DATA = test_session_hooks--1.0.sql
-
-REGRESS = test_session_hooks
 REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
 
 ifdef USE_PGXS
@@ -20,6 +16,31 @@ include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif
 
-# override installcheck - this module requires preloading the test module
-installcheck:
-	@echo Cannot run $@ for test_session_hooks. Run "'make check'" instead.
+# Disabled because these tests require shared_preload_libraries to be
+# set but users unlikely have that set up.
+installcheck:;
+
+# But it can nonetheless be very helpful to run tests on preexisting
+# installation, allow to do so, but only if requested explicitly.
+installcheck-force: regresscheck-install-force
+
+check: regresscheck
+
+submake-regress:
+	$(MAKE) -C $(top_builddir)/src/test/regress all
+
+submake-test_session_hooks:
+	$(MAKE) -C $(top_builddir)/src/test/modules/test_session_hooks
+
+REGRESSCHECKS = test_session_hooks
+
+regresscheck: | submake-regress submake-test_session_hooks temp-install
+	$(pg_regress_check) $(REGRESS_OPTS) $(REGRESSCHECKS)
+
+regresscheck-install-force: | submake-regress submake-test_decoding temp-install
+	$(pg_regress_installcheck) $(REGRESSCHECKS)
+
+.PHONY: submake-test_decoding submake-regress check \
+	regresscheck regresscheck-install-force
+
+temp-install: EXTRA_INSTALL=src/test/modules/test_session_hooks
diff --git a/src/test/modules/test_session_hooks/expected/test_session_hooks.out b/src/test/modules/test_session_hooks/expected/test_session_hooks.out
index be1b94953c..8c1a92f7e8 100644
--- a/src/test/modules/test_session_hooks/expected/test_session_hooks.out
+++ b/src/test/modules/test_session_hooks/expected/test_session_hooks.out
@@ -16,16 +16,18 @@ SELECT * FROM session_hook_log ORDER BY id;
 
 \c :prevdb regress_sess_hook_usr2
 SELECT * FROM session_hook_log ORDER BY id;
- id |       dbname       |        username        | hook_at 
-----+--------------------+------------------------+---------
-  1 | contrib_regression | regress_sess_hook_usr2 | START
+ id |   dbname   |        username        | hook_at 
+----+------------+------------------------+---------
+  1 | regression | regress_sess_hook_usr2 | START
 (1 row)
 
 \c :prevdb :prevusr
 SELECT * FROM session_hook_log ORDER BY id;
- id |       dbname       |        username        | hook_at 
-----+--------------------+------------------------+---------
-  1 | contrib_regression | regress_sess_hook_usr2 | START
-  2 | contrib_regression | regress_sess_hook_usr2 | END
+ id |   dbname   |        username        | hook_at 
+----+------------+------------------------+---------
+  1 | regression | regress_sess_hook_usr2 | START
+  2 | regression | regress_sess_hook_usr2 | END
 (2 rows)
 
+DROP ROLE regress_sess_hook_usr1;
+DROP ROLE regress_sess_hook_usr2;
diff --git a/src/test/modules/test_session_hooks/sql/test_session_hooks.sql b/src/test/modules/test_session_hooks/sql/test_session_hooks.sql
index 5e0864753d..9644c5a694 100644
--- a/src/test/modules/test_session_hooks/sql/test_session_hooks.sql
+++ b/src/test/modules/test_session_hooks/sql/test_session_hooks.sql
@@ -10,3 +10,5 @@ SELECT * FROM session_hook_log ORDER BY id;
 SELECT * FROM session_hook_log ORDER BY id;
 \c :prevdb :prevusr
 SELECT * FROM session_hook_log ORDER BY id;
+DROP ROLE regress_sess_hook_usr1;
+DROP ROLE regress_sess_hook_usr2;
diff --git a/src/test/modules/test_session_hooks/test_session_hooks--1.0.sql b/src/test/modules/test_session_hooks/test_session_hooks--1.0.sql
deleted file mode 100644
index 16bcee9882..0000000000
--- a/src/test/modules/test_session_hooks/test_session_hooks--1.0.sql
+++ /dev/null
@@ -1,4 +0,0 @@
-/* src/test/modules/test_hook_session/test_hook_session--1.0.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION test_hook_session" to load this file. \quit
diff --git a/src/test/modules/test_session_hooks/test_session_hooks.c b/src/test/modules/test_session_hooks/test_session_hooks.c
index 4e2eef183e..04acd27eeb 100644
--- a/src/test/modules/test_session_hooks/test_session_hooks.c
+++ b/src/test/modules/test_session_hooks/test_session_hooks.c
@@ -72,7 +72,7 @@ register_session_hook(const char *hook_at)
 
 /* sample session start hook function */
 static void
-sample_session_start_hook()
+sample_session_start_hook(void)
 {
 	/* Hook just normal backends */
 	if (MyBackendId != InvalidBackendId)
@@ -86,7 +86,7 @@ sample_session_start_hook()
 
 /* sample session end hook function */
 static void
-sample_session_end_hook()
+sample_session_end_hook(void)
 {
 	/* Hook just normal backends */
 	if (MyBackendId != InvalidBackendId)
diff --git a/src/test/modules/test_session_hooks/test_session_hooks.control b/src/test/modules/test_session_hooks/test_session_hooks.control
deleted file mode 100644
index 7d7ef9f3f4..0000000000
--- a/src/test/modules/test_session_hooks/test_session_hooks.control
+++ /dev/null
@@ -1,3 +0,0 @@
-comment = 'Test start/end hook session with an extension'
-default_version = '1.0'
-relocatable = true

Ashutosh Sharma

ashu.coek88@gmail.com

about 8 years ago

In reply to: Michael Paquier (#4)

Re: pgsql: Disable installcheck tests for test_session_hooks

Hi Michael,

On Thu, Nov 16, 2017 at 6:44 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Nov 16, 2017 at 3:45 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Nov 16, 2017 at 11:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Disable installcheck tests for test_session_hooks

This seems not quite right; it's causing warnings like this:

$ make -s clean
Makefile:25: warning: overriding commands for target `installcheck'
../../../../src/makefiles/pgxs.mk:275: warning: ignoring old commands for target `installcheck'

This is caused by the fact that installcheck gets defined because of
REGRESS. This stuff should do something similar to what is done in
contrib/test_decoding/Makefile, and I think that it would be useful to
have the option to force installcheck on an installation that has all
the needed parameters set. With this last point, I think that the
test_session_hooks.sql should drop the roles it created. Attached is a
cleanup patch (includes fixes for compilation warnings).

Just sending an updated patch to cope with the fact that this module
does not need to define the EXTENSION and DATA fields, and does not
need a sql and a control file..

While running make check with 'session_hook_makefile_v2.patch'
applied, i could see the background worker (logical replication
process) getting aborted due to assertion failure. Here are the logs
observed in 'logs/postmaster.log' file.

2017-11-16 19:06:21.748 IST [65382] LOG: checkpoint complete: wrote 3
buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.000 s, sync=0.000 s, total=0.001 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=1 kB, estimate=1 kB
2017-11-16 19:06:21.869 IST [65382] LOG: checkpoint starting:
immediate force wait
2017-11-16 19:06:21.870 IST [65382] LOG: checkpoint complete: wrote 0
buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.000 s, sync=0.000 s, total=0.000 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=0 kB, estimate=1 kB
2017-11-16 19:06:22.059 IST [65378] LOG: received fast shutdown request
2017-11-16 19:06:22.059 IST [65378] LOG: aborting any active transactions
TRAP: BadState("!(((bool) ((CurrentUserId) != ((Oid) 0))))", File:
"miscinit.c", Line: 286)
2017-11-16 19:06:22.090 IST [65378] LOG: background worker "logical
replication launcher" (PID 65387) exited with exit code 1
2017-11-16 19:06:22.337 IST [65378] LOG: autovacuum launcher process
(PID 65385) was terminated by signal 6: Aborted
2017-11-16 19:06:22.337 IST [65378] LOG: terminating any other active
server processes

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Andrew Dunstan

andrew.dunstan@2ndquadrant.com

about 8 years ago

In reply to: Ashutosh Sharma (#5)

Re: pgsql: Disable installcheck tests for test_session_hooks

On 11/16/2017 09:14 AM, Ashutosh Sharma wrote:

Just sending an updated patch to cope with the fact that this module
does not need to define the EXTENSION and DATA fields, and does not
need a sql and a control file..

While running make check with 'session_hook_makefile_v2.patch'
applied, i could see the background worker (logical replication
process) getting aborted due to assertion failure. Here are the logs
observed in 'logs/postmaster.log' file.

2017-11-16 19:06:21.748 IST [65382] LOG: checkpoint complete: wrote 3
buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.000 s, sync=0.000 s, total=0.001 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=1 kB, estimate=1 kB
2017-11-16 19:06:21.869 IST [65382] LOG: checkpoint starting:
immediate force wait
2017-11-16 19:06:21.870 IST [65382] LOG: checkpoint complete: wrote 0
buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.000 s, sync=0.000 s, total=0.000 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=0 kB, estimate=1 kB
2017-11-16 19:06:22.059 IST [65378] LOG: received fast shutdown request
2017-11-16 19:06:22.059 IST [65378] LOG: aborting any active transactions
TRAP: BadState("!(((bool) ((CurrentUserId) != ((Oid) 0))))", File:
"miscinit.c", Line: 286)
2017-11-16 19:06:22.090 IST [65378] LOG: background worker "logical
replication launcher" (PID 65387) exited with exit code 1
2017-11-16 19:06:22.337 IST [65378] LOG: autovacuum launcher process
(PID 65385) was terminated by signal 6: Aborted
2017-11-16 19:06:22.337 IST [65378] LOG: terminating any other active
server processes

Ugh. I'm just going to revert the whole thing for now until we sort out
all the issues.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Fabrízio de Royes Mello

fabriziomello@gmail.com

about 8 years ago

In reply to: Andrew Dunstan (#6)

Re: pgsql: Disable installcheck tests for test_session_hooks

On Thu, Nov 16, 2017 at 2:21 PM, Andrew Dunstan <
andrew.dunstan@2ndquadrant.com> wrote:

On 11/16/2017 09:14 AM, Ashutosh Sharma wrote:

Just sending an updated patch to cope with the fact that this module
does not need to define the EXTENSION and DATA fields, and does not
need a sql and a control file..

While running make check with 'session_hook_makefile_v2.patch'
applied, i could see the background worker (logical replication
process) getting aborted due to assertion failure. Here are the logs
observed in 'logs/postmaster.log' file.

2017-11-16 19:06:21.748 IST [65382] LOG: checkpoint complete: wrote 3
buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.000 s, sync=0.000 s, total=0.001 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=1 kB, estimate=1 kB
2017-11-16 19:06:21.869 IST [65382] LOG: checkpoint starting:
immediate force wait
2017-11-16 19:06:21.870 IST [65382] LOG: checkpoint complete: wrote 0
buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.000 s, sync=0.000 s, total=0.000 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=0 kB, estimate=1 kB
2017-11-16 19:06:22.059 IST [65378] LOG: received fast shutdown request
2017-11-16 19:06:22.059 IST [65378] LOG: aborting any active

transactions

TRAP: BadState("!(((bool) ((CurrentUserId) != ((Oid) 0))))", File:
"miscinit.c", Line: 286)
2017-11-16 19:06:22.090 IST [65378] LOG: background worker "logical
replication launcher" (PID 65387) exited with exit code 1
2017-11-16 19:06:22.337 IST [65378] LOG: autovacuum launcher process
(PID 65385) was terminated by signal 6: Aborted
2017-11-16 19:06:22.337 IST [65378] LOG: terminating any other active
server processes

Ugh. I'm just going to revert the whole thing for now until we sort out
all the issues.

I'm looking into it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

Timbira: http://www.timbira.com.br
Blog: http://fabriziomello.github.io
Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Github: http://github.com/fabriziomello