pgsql: Change the way that LWLocks for extensions are allocated.
Change the way that LWLocks for extensions are allocated.
The previous RequestAddinLWLocks() method had several disadvantages.
First, the locks would be in the main tranche; we've recently decided
that it's useful for LWLocks used for separate purposes to have
separate tranche IDs. Second, there wasn't any correlation between
what code called RequestAddinLWLocks() and what code called
LWLockAssign(); when multiple modules are in use, it could become
quite difficult to troubleshoot problems where LWLockAssign() ran out
of locks. To fix, create a concept of named LWLock tranches which
can be used either by extension or by core code.
Amit Kapila and Robert Haas
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/c1772ad9225641c921545b35c84ee478c326b95e
Modified Files
--------------
contrib/pg_stat_statements/pg_stat_statements.c | 4 +-
doc/src/sgml/xfunc.sgml | 9 +-
src/backend/postmaster/postmaster.c | 6 +
src/backend/storage/lmgr/lwlock.c | 212 ++++++++++++++++++++----
src/include/pg_config_manual.h | 5 -
src/include/storage/lwlock.h | 22 ++-
src/tools/pgindent/typedefs.list | 2 +
7 files changed, 210 insertions(+), 50 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Hi,
On 2016-02-04 21:43:14 +0000, Robert Haas wrote:
Change the way that LWLocks for extensions are allocated.
The previous RequestAddinLWLocks() method had several disadvantages.
First, the locks would be in the main tranche; we've recently decided
that it's useful for LWLocks used for separate purposes to have
separate tranche IDs. Second, there wasn't any correlation between
what code called RequestAddinLWLocks() and what code called
LWLockAssign(); when multiple modules are in use, it could become
quite difficult to troubleshoot problems where LWLockAssign() ran out
of locks. To fix, create a concept of named LWLock tranches which
can be used either by extension or by core code.Amit Kapila and Robert Haas
I noticed that this code has no test coverage:
http://coverage.postgresql.org/src/backend/storage/lmgr/lwlock.c.gcov.html
It'd be good to add some, although I'm not entirely sure what the best
way is. Maybe add a simple pg_stat_statements test?
Andres
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Sat, Aug 27, 2016 at 3:30 AM, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2016-02-04 21:43:14 +0000, Robert Haas wrote:
Change the way that LWLocks for extensions are allocated.
The previous RequestAddinLWLocks() method had several disadvantages.
First, the locks would be in the main tranche; we've recently decided
that it's useful for LWLocks used for separate purposes to have
separate tranche IDs. Second, there wasn't any correlation between
what code called RequestAddinLWLocks() and what code called
LWLockAssign(); when multiple modules are in use, it could become
quite difficult to troubleshoot problems where LWLockAssign() ran out
of locks. To fix, create a concept of named LWLock tranches which
can be used either by extension or by core code.Amit Kapila and Robert Haas
I noticed that this code has no test coverage:
http://coverage.postgresql.org/src/backend/storage/lmgr/lwlock.c.gcov.html
It'd be good to add some, although I'm not entirely sure what the best
way is. Maybe add a simple pg_stat_statements test?
That would also have the advantage of improving the test coverage for
pg_stat_statements, which is at the moment quite a bit thinner than
the coverage for lwlock.c.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas wrote:
That would also have the advantage of improving the test coverage for
pg_stat_statements, which is at the moment quite a bit thinner than
the coverage for lwlock.c.
Hmm, the coverage-html report is not currently covering contrib ... I
suppose that's an easily fixable oversight.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 29, 2016 at 10:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Aug 27, 2016 at 3:30 AM, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2016-02-04 21:43:14 +0000, Robert Haas wrote:
Change the way that LWLocks for extensions are allocated.
The previous RequestAddinLWLocks() method had several disadvantages.
First, the locks would be in the main tranche; we've recently decided
that it's useful for LWLocks used for separate purposes to have
separate tranche IDs. Second, there wasn't any correlation between
what code called RequestAddinLWLocks() and what code called
LWLockAssign(); when multiple modules are in use, it could become
quite difficult to troubleshoot problems where LWLockAssign() ran out
of locks. To fix, create a concept of named LWLock tranches which
can be used either by extension or by core code.Amit Kapila and Robert Haas
I noticed that this code has no test coverage:
http://coverage.postgresql.org/src/backend/storage/lmgr/lwlock.c.gcov.html
It'd be good to add some, although I'm not entirely sure what the best
way is. Maybe add a simple pg_stat_statements test?That would also have the advantage of improving the test coverage for
pg_stat_statements, which is at the moment quite a bit thinner than
the coverage for lwlock.c.
I will write such a test case either in this week or early next week.
I hope this is not super urgent, let me know if you think otherwise.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
I will write such a test case either in this week or early next week.
Great.
I hope this is not super urgent, let me know if you think otherwise.
It's not urgent, no.
Thanks!
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
I will write such a test case either in this week or early next week.
Great.
Okay, attached patch adds some simple tests for pg_stat_statements.
One thing to note is that we can't add pg_stat_statements tests for
installcheck as this module requires shared_preload_libraries to be
set. Hope this suffices the need.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
test_pg_stat_statements-v1.patchapplication/octet-stream; name=test_pg_stat_statements-v1.patchDownload
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index ddcdb10..8f67c6a 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -11,6 +11,10 @@ PGFILEDESC = "pg_stat_statements - execution statistics of SQL statements"
LDFLAGS_SL += $(filter -lm, $(LIBS))
+# Note: because we don't tell the Makefile there are any regression tests,
+# we have to clean those result files explicitly
+EXTRA_CLEAN = $(pg_regress_clean_files) ./regression_output
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -21,3 +25,29 @@ top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
+
+# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+installcheck:;
+
+check: regresscheck
+
+submake-regress:
+ $(MAKE) -C $(top_builddir)/src/test/regress all
+
+submake-pg_stat_statements:
+ $(MAKE) -C $(top_builddir)/contrib/pg_stat_statements
+
+REGRESSCHECKS=pg_stat_statements
+
+regresscheck: | submake-regress submake-pg_stat_statements temp-install
+ $(MKDIR_P) regression_output
+ $(pg_regress_check) \
+ --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf \
+ --temp-instance=./tmp_check \
+ --outputdir=./regression_output \
+ $(REGRESSCHECKS)
+
+.PHONY: submake-pg_stat_statements submake_regress check regresscheck
+
+temp-install: EXTRA_INSTALL=contrib/pg_stat_statements
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
new file mode 100644
index 0000000..decb226
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -0,0 +1,21 @@
+CREATE EXTENSION pg_stat_statements;
+CREATE TABLE test (a int, b char(20));
+-- test the basic functionality of pg_stat_statements
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+INSERT INTO test VALUES(generate_series(1, 10), 'aaa');
+UPDATE test SET b = 'bbb' WHERE a > 5;
+SELECT queryid, query, calls, rows from pg_stat_statements ORDER BY rows;
+ queryid | query | calls | rows
+------------+----------------------------------------------------+-------+------
+ 190868975 | SELECT pg_stat_statements_reset(); | 1 | 1
+ 1181103097 | UPDATE test SET b = ? WHERE a > ?; | 1 | 5
+ 2522140409 | INSERT INTO test VALUES(generate_series(?, ?), ?); | 1 | 10
+(3 rows)
+
+DROP TABLE test;
+DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.conf
new file mode 100644
index 0000000..13346e2
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements.conf
@@ -0,0 +1 @@
+shared_preload_libraries = 'pg_stat_statements'
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
new file mode 100644
index 0000000..2f16cb5
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -0,0 +1,15 @@
+CREATE EXTENSION pg_stat_statements;
+
+CREATE TABLE test (a int, b char(20));
+
+-- test the basic functionality of pg_stat_statements
+SELECT pg_stat_statements_reset();
+
+INSERT INTO test VALUES(generate_series(1, 10), 'aaa');
+UPDATE test SET b = 'bbb' WHERE a > 5;
+
+SELECT queryid, query, calls, rows from pg_stat_statements ORDER BY rows;
+
+DROP TABLE test;
+
+DROP EXTENSION pg_stat_statements;
On Sat, Sep 10, 2016 at 5:02 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
I will write such a test case either in this week or early next week.
Great.
Okay, attached patch adds some simple tests for pg_stat_statements.
One thing to note is that we can't add pg_stat_statements tests for
installcheck as this module requires shared_preload_libraries to be
set. Hope this suffices the need.
Registered this patch for next CF.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
I have started with the review for this patch and would like to share
some of my initial review comments that requires author's attention.
1) I am getting some trailing whitespace errors when trying to apply
this patch using git apply command. Following are the error messages i
am getting.
[edb@localhost postgresql]$ git apply test_pg_stat_statements-v1.patch
test_pg_stat_statements-v1.patch:28: trailing whitespace.
$(MAKE) -C $(top_builddir)/src/test/regress all
test_pg_stat_statements-v1.patch:41: space before tab in indent.
$(REGRESSCHECKS)
warning: 2 lines add whitespace errors.
2) The test-case you have added is failing that is because queryid is
going to be different every time you execute the test-case. I think it
would be better to remove the queryid column from "SELECT queryid,
query, calls, rows from pg_stat_statements ORDER BY rows". Below is
the output i got upon test-case execution.
make regresscheck
============== running regression test queries ==============
test pg_stat_statements ... FAILED
============== shutting down postmaster ==============
3) Ok. As you mentioned upthread, I do agree with the fact that we
can't add pg_stat_statements tests for installcheck as this module
requires shared_preload_libraries to be set. But, if there is an
already existing installation with pg_stat_statements module loaded we
should allow the test to run on this installation if requested
explicitly by the user. I think we can add some target like
'installcheck-force' in the MakeFile that would help the end users to
run the test on his own installation. Thoughts?
Also, I am changed the status in the commit-fest from "Needs review"
to "waiting on Author".
On Mon, Sep 19, 2016 at 6:26 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Sep 10, 2016 at 5:02 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
I will write such a test case either in this week or early next week.
Great.
Okay, attached patch adds some simple tests for pg_stat_statements.
One thing to note is that we can't add pg_stat_statements tests for
installcheck as this module requires shared_preload_libraries to be
set. Hope this suffices the need.Registered this patch for next CF.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 2, 2016 at 12:48 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
Hi,
I have started with the review for this patch and would like to share
some of my initial review comments that requires author's attention.
Thanks.
1) I am getting some trailing whitespace errors when trying to apply
this patch using git apply command. Following are the error messages i
am getting.[edb@localhost postgresql]$ git apply test_pg_stat_statements-v1.patch
test_pg_stat_statements-v1.patch:28: trailing whitespace.
$(MAKE) -C $(top_builddir)/src/test/regress all
test_pg_stat_statements-v1.patch:41: space before tab in indent.
$(REGRESSCHECKS)
warning: 2 lines add whitespace errors.
Fixed.
2) The test-case you have added is failing that is because queryid is
going to be different every time you execute the test-case.
It shouldn't be different each time you execute test as this is a hash
code, but I agree that it is not stable and it can vary across
platforms, so removed it from test.
3) Ok. As you mentioned upthread, I do agree with the fact that we
can't add pg_stat_statements tests for installcheck as this module
requires shared_preload_libraries to be set. But, if there is an
already existing installation with pg_stat_statements module loaded we
should allow the test to run on this installation if requested
explicitly by the user. I think we can add some target like
'installcheck-force' in the MakeFile that would help the end users to
run the test on his own installation.
I had also thought about it while preparing patch, but I couldn't find
any clear use case. I think it could be useful during development of
a module, but not sure if it is worth to add a target. I think there
is no harm in adding such a target, but lets take an opinion from
committer or others as well before adding this target. What do you
say?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
test_pg_stat_statements-v2.patchapplication/octet-stream; name=test_pg_stat_statements-v2.patchDownload
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index ddcdb10..2557e0d 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -11,6 +11,10 @@ PGFILEDESC = "pg_stat_statements - execution statistics of SQL statements"
LDFLAGS_SL += $(filter -lm, $(LIBS))
+# Note: because we don't tell the Makefile there are any regression tests,
+# we have to clean those result files explicitly
+EXTRA_CLEAN = $(pg_regress_clean_files) ./regression_output
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -21,3 +25,29 @@ top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
+
+# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+installcheck:;
+
+check: regresscheck
+
+submake-regress:
+ $(MAKE) -C $(top_builddir)/src/test/regress all
+
+submake-pg_stat_statements:
+ $(MAKE) -C $(top_builddir)/contrib/pg_stat_statements
+
+REGRESSCHECKS=pg_stat_statements
+
+regresscheck: | submake-regress submake-pg_stat_statements temp-install
+ $(MKDIR_P) regression_output
+ $(pg_regress_check) \
+ --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf \
+ --temp-instance=./tmp_check \
+ --outputdir=./regression_output \
+ $(REGRESSCHECKS)
+
+.PHONY: submake-pg_stat_statements submake_regress check regresscheck
+
+temp-install: EXTRA_INSTALL=contrib/pg_stat_statements
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
new file mode 100644
index 0000000..3573c19
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -0,0 +1,21 @@
+CREATE EXTENSION pg_stat_statements;
+CREATE TABLE test (a int, b char(20));
+-- test the basic functionality of pg_stat_statements
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+INSERT INTO test VALUES(generate_series(1, 10), 'aaa');
+UPDATE test SET b = 'bbb' WHERE a > 5;
+SELECT query, calls, rows from pg_stat_statements ORDER BY rows;
+ query | calls | rows
+----------------------------------------------------+-------+------
+ SELECT pg_stat_statements_reset(); | 1 | 1
+ UPDATE test SET b = ? WHERE a > ?; | 1 | 5
+ INSERT INTO test VALUES(generate_series(?, ?), ?); | 1 | 10
+(3 rows)
+
+DROP TABLE test;
+DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.conf
new file mode 100644
index 0000000..13346e2
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements.conf
@@ -0,0 +1 @@
+shared_preload_libraries = 'pg_stat_statements'
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
new file mode 100644
index 0000000..7e2b263
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -0,0 +1,15 @@
+CREATE EXTENSION pg_stat_statements;
+
+CREATE TABLE test (a int, b char(20));
+
+-- test the basic functionality of pg_stat_statements
+SELECT pg_stat_statements_reset();
+
+INSERT INTO test VALUES(generate_series(1, 10), 'aaa');
+UPDATE test SET b = 'bbb' WHERE a > 5;
+
+SELECT query, calls, rows from pg_stat_statements ORDER BY rows;
+
+DROP TABLE test;
+
+DROP EXTENSION pg_stat_statements;
Hi,
I had also thought about it while preparing patch, but I couldn't find
any clear use case. I think it could be useful during development of
a module, but not sure if it is worth to add a target. I think there
is no harm in adding such a target, but lets take an opinion from
committer or others as well before adding this target. What do you
say?
Ok. We can do that.
I have verified the updated v2 patch. The patch looks good to me. I am
marking it as ready for committer. Thanks.
With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 8, 2016 at 4:23 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
Hi,
I had also thought about it while preparing patch, but I couldn't find
any clear use case. I think it could be useful during development of
a module, but not sure if it is worth to add a target. I think there
is no harm in adding such a target, but lets take an opinion from
committer or others as well before adding this target. What do you
say?Ok. We can do that.
I have verified the updated v2 patch. The patch looks good to me. I am
marking it as ready for committer.
Thanks for the review.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
On 2016-11-08 10:25:11 +0530, Amit Kapila wrote:
ifdef USE_PGXS PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) @@ -21,3 +25,29 @@ top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif + +# Disabled because these tests require "shared_preload_libraries=pg_stat_statements", +# which typical installcheck users do not have (e.g. buildfarm clients).
+installcheck:; + +check: regresscheck + +submake-regress: + $(MAKE) -C $(top_builddir)/src/test/regress all + +submake-pg_stat_statements: + $(MAKE) -C $(top_builddir)/contrib/pg_stat_statements
Why is this a submake? That seems to make little sense? But stepping
back one step further: I don't think we need all the remade rules in the
first place.
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
and then
installcheck: REGRESS=
to prevent installcheck from doing anything ought to be enough these days.
Committed after simplifying the Makefile.
We can't simplify test_decoding's makefile to that extent, because it
uses isolationtester, which we don't provide for contrib yet...
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
Committed after simplifying the Makefile.
While I have no particular objection to adding these tests, the
commit log's claim that this will improve buildfarm testing is
quite wrong. The buildfarm runs "make installcheck" not
"make check" in contrib. What I'm actually seeing in the
buildfarm reports is
make -C pg_stat_statements installcheck
make -C ../../src/test/regress pg_regress
make -C ../../../src/port all
make -C ../backend submake-errcodes
make[4]: Nothing to be done for `submake-errcodes'.
make -C ../../../src/common all
make -C ../backend submake-errcodes
make[4]: Nothing to be done for `submake-errcodes'.
../../src/test/regress/pg_regress --inputdir=. --bindir='/Users/buildfarm/bf-data/HEAD/inst/bin' --port=5678 --temp-config ../../contrib/pg_stat_statements/pg_stat_statements.conf --dbname=contrib_regression_pg_stat_statements
(using postmaster on Unix socket, port 5678)
============== dropping database "contrib_regression_pg_stat_statements" ==============
NOTICE: database "contrib_regression_pg_stat_statements" does not exist, skipping
DROP DATABASE
============== creating database "contrib_regression_pg_stat_statements" ==============
CREATE DATABASE
ALTER DATABASE
============== running regression test queries ==============
=====================
All 0 tests passed.
=====================
which is a rather blatant waste of cycles. I would suggest an explicit
do-nothing installcheck rule rather than the hack you came up with here.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
Committed after simplifying the Makefile.
While I have no particular objection to adding these tests, the
commit log's claim that this will improve buildfarm testing is
quite wrong. The buildfarm runs "make installcheck" not
"make check" in contrib.
Gah. It was more aimed at the coverage stuff, but that might work the
same. Alvaro?
which is a rather blatant waste of cycles. I would suggest an explicit
do-nothing installcheck rule rather than the hack you came up with here.
I had that at first, but that generates a warning about overwriting the
makefile target - which afaics cannot be fixed.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
which is a rather blatant waste of cycles. I would suggest an explicit
do-nothing installcheck rule rather than the hack you came up with here.
I had that at first, but that generates a warning about overwriting the
makefile target - which afaics cannot be fixed.
Hm. What about inventing an additional macro NO_INSTALLCHECK that
prevents pgxs.mk from generating an installcheck rule? It's not
like we don't have similar issues elsewhere, eg contrib/sepgsql.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
which is a rather blatant waste of cycles. I would suggest an explicit
do-nothing installcheck rule rather than the hack you came up with here.I had that at first, but that generates a warning about overwriting the
makefile target - which afaics cannot be fixed.Hm. What about inventing an additional macro NO_INSTALLCHECK that
prevents pgxs.mk from generating an installcheck rule?
That'd work. I'd also be ok with living with the warning. I have to say
I find it hard to be concerned about the cycles here. It's not like
anybody complained about make check unconditionally creating a test
installation, even if there's tests in a contrib module...
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
Hm. What about inventing an additional macro NO_INSTALLCHECK that
prevents pgxs.mk from generating an installcheck rule?
That'd work. I'd also be ok with living with the warning. I have to say
I find it hard to be concerned about the cycles here. It's not like
anybody complained about make check unconditionally creating a test
installation, even if there's tests in a contrib module...
[ shrug... ] The reason why the buildfarm doesn't do "make check" here is
precisely the extra cycles it would take. Those add up over hundreds of
runs, particularly on slow machines. So I'm not excited about running
completely useless operations.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
which is a rather blatant waste of cycles. I would suggest an explicit
do-nothing installcheck rule rather than the hack you came up with here.I had that at first, but that generates a warning about overwriting the
makefile target - which afaics cannot be fixed.Hm. What about inventing an additional macro NO_INSTALLCHECK that
prevents pgxs.mk from generating an installcheck rule? It's not
like we don't have similar issues elsewhere, eg contrib/sepgsql.
Well, that one seems a bit different. Seems easy enough to do. Do we
want to make that macro that prevents installcheck from being defined,
or one that forces it to be empty? The former has the disadvantage that
one has to be careful to define a target, to avoid breaking recursion
from the upper levels.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-11-14 12:14:10 -0800, Andres Freund wrote:
On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
which is a rather blatant waste of cycles. I would suggest an explicit
do-nothing installcheck rule rather than the hack you came up with here.I had that at first, but that generates a warning about overwriting the
makefile target - which afaics cannot be fixed.Hm. What about inventing an additional macro NO_INSTALLCHECK that
prevents pgxs.mk from generating an installcheck rule? It's not
like we don't have similar issues elsewhere, eg contrib/sepgsql.Well, that one seems a bit different. Seems easy enough to do. Do we
want to make that macro that prevents installcheck from being defined,
or one that forces it to be empty? The former has the disadvantage that
one has to be careful to define a target, to avoid breaking recursion
from the upper levels.
Oh, that disadvantage doesn't actually exist, because installcheck is a
.PHONY target...
I've for now not added a variant that prevents plain 'make check' from
doing anything. I guess it could be useful when a contrib module wants
to run tests via something else than pg_regress?
Patch attached.
Andres
Attachments:
0001-Provide-NO_INSTALLCHECK-for-pgxs.patchtext/x-patch; charset=us-asciiDownload
From 906051a23831c5d337556e56d19962f2e8444457 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 14 Nov 2016 12:29:30 -0800
Subject: [PATCH] Provide NO_INSTALLCHECK for pgxs.
This allows us to avoid running the regression tests in contrib modules
like pg_stat_statement in a less ugly manner.
Discussion: <22432.1478968242@sss.pgh.pa.us>
---
contrib/pg_stat_statements/Makefile | 7 +++----
doc/src/sgml/extend.sgml | 9 +++++++++
src/makefiles/pgxs.mk | 4 ++++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index f1a45eb..298951a 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -13,6 +13,9 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
REGRESS = pg_stat_statements
+# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
@@ -24,7 +27,3 @@ top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
-
-# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
-# which typical installcheck users do not have (e.g. buildfarm clients).
-installcheck: REGRESS=
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index e19c657..f9d91a3 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1194,6 +1194,15 @@ include $(PGXS)
</varlistentry>
<varlistentry>
+ <term><varname>NO_INSTALLCHECK</varname></term>
+ <listitem>
+ <para>
+ don't define an installcheck target, useful e.g. if tests require special configuration, or don't use pg_regress
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><varname>EXTRA_CLEAN</varname></term>
<listitem>
<para>
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 2b4d684..c27004e 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -40,6 +40,8 @@
# which need to be built first
# REGRESS -- list of regression test cases (without suffix)
# REGRESS_OPTS -- additional switches to pass to pg_regress
+# NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
+# tests require special configuration, or don't use pg_regress
# EXTRA_CLEAN -- extra files to remove in 'make clean'
# PG_CPPFLAGS -- will be added to CPPFLAGS
# PG_LIBS -- will be added to PROGRAM link line
@@ -268,8 +270,10 @@ ifndef PGXS
endif
# against installed postmaster
+ifndef NO_INSTALLCHECK
installcheck: submake $(REGRESS_PREP)
$(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS)
+endif
ifdef PGXS
check:
--
2.10.2
Andres Freund wrote:
On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
Committed after simplifying the Makefile.
While I have no particular objection to adding these tests, the
commit log's claim that this will improve buildfarm testing is
quite wrong. The buildfarm runs "make installcheck" not
"make check" in contrib.Gah. It was more aimed at the coverage stuff, but that might work the
same. Alvaro?
Already replied on IM, but for the record, coverage.postgresql.org
currently runs this:
make -j4 >> $LOG 2>&1
make check-world >> $LOG 2>&1
make -C src/test/ssl check >> $LOG 2>&1
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers