On 2017-03-11 11:48:31 -0800, Andres Freund wrote: > On 2017-03-11 12:05:23 -0500, Tom Lane wrote: > > I wrote: > > > I believe the core problem is that contrib/test_decoding's regresscheck > > > and isolationcheck targets each want to use ./tmp_check as their > > > --temp-instance. make has no reason to believe it shouldn't run those > > > two sub-jobs in parallel, but when it does, we get two postmasters trying > > > to share the same directory. This looks reasonably straightforward to > > > solve, but I'm not entirely familiar with the code here, and am not > > > sure what is the least ugly way to fix it. > > > > Enlarging on that: if I cd into contrib/test_decoding and do > > "make check -j4" or so, it reliably fails. > > Yep, can reproduce here as well. Interesting that, with -j16, I could > survive several dozen runs from the toplevel locally. > > > > This is a localized patch that only fixes things for > > contrib/test_decoding; what I'm wondering is if it would be better to > > establish a more widespread convention that > > $(pg_isolation_regress_check) should use a different --temp-instance > > directory than $(pg_regress_check) does. > > I think that'd be a good plan. We probably should also keep --outputdir > seperate (which test_decoding/Makefile does, but > pg_isolation_regress_check doesn't)?
Here's a patch doing that (based on yours). I'd be kind of inclined to set --outputdir for !isolation tests too; possibly even move tmp_check below output_iso/ output_regress/ or such - but that seems like it potentially could cause some disagreement... - Andres
>From fb89f431d120e9123dca367d23f330b7d31b3f01 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sat, 11 Mar 2017 15:03:18 -0800 Subject: [PATCH] Improve isolation regression tests infrastructure. Previously if a directory had both isolationtester and plain regression tests they couldn't be run in parallel, because they'd access the same files/directories. That, so far, only affected contrib/test_decoding. Rather than fix that locally in contrib/test_decoding improve pg_regress_isolation_[install]check to use separate resources from plain regression tests. Use the improved helpers everywhere, even where previously not used. Author: Tom Lane and Andres Freund Discussion: https://postgr.es/m/20170311194831.vm5ikpczq52c2...@alap3.anarazel.de --- contrib/test_decoding/.gitignore | 5 +++-- contrib/test_decoding/Makefile | 7 +------ src/Makefile.global.in | 29 ++++++++++++++++++++++------ src/test/isolation/.gitignore | 5 ++--- src/test/isolation/Makefile | 8 ++++---- src/test/modules/snapshot_too_old/.gitignore | 2 +- src/test/modules/snapshot_too_old/Makefile | 6 +++--- 7 files changed, 37 insertions(+), 25 deletions(-) diff --git a/contrib/test_decoding/.gitignore b/contrib/test_decoding/.gitignore index 1f95503494..b4903eba65 100644 --- a/contrib/test_decoding/.gitignore +++ b/contrib/test_decoding/.gitignore @@ -1,5 +1,6 @@ # Generated subdirectories /log/ -/isolation_output/ -/regression_output/ +/results/ +/output_iso/ /tmp_check/ +/tmp_check_iso/ diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile index d2bc8b8350..6c18189d9d 100644 --- a/contrib/test_decoding/Makefile +++ b/contrib/test_decoding/Makefile @@ -5,7 +5,7 @@ PGFILEDESC = "test_decoding - example of a logical decoding output plugin" # 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 ./isolation_output +EXTRA_CLEAN = $(pg_regress_clean_files) ifdef USE_PGXS PG_CONFIG = pg_config @@ -42,11 +42,8 @@ REGRESSCHECKS=ddl xact rewrite toast permissions decoding_in_xact \ spill slot regresscheck: | submake-regress submake-test_decoding temp-install - $(MKDIR_P) regression_output $(pg_regress_check) \ --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf \ - --temp-instance=./tmp_check \ - --outputdir=./regression_output \ $(REGRESSCHECKS) regresscheck-install-force: | submake-regress submake-test_decoding temp-install @@ -56,10 +53,8 @@ regresscheck-install-force: | submake-regress submake-test_decoding temp-install ISOLATIONCHECKS=mxact delayed_startup ondisk_startup concurrent_ddl_dml isolationcheck: | submake-isolation submake-test_decoding temp-install - $(MKDIR_P) isolation_output $(pg_isolation_regress_check) \ --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf \ - --outputdir=./isolation_output \ $(ISOLATIONCHECKS) isolationcheck-install-force: all | submake-isolation submake-test_decoding temp-install diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 831d39a9d1..9796ffd753 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -545,14 +545,31 @@ TEMP_CONF += --temp-config=$(TEMP_CONFIG) endif pg_regress_locale_flags = $(if $(ENCODING),--encoding=$(ENCODING)) $(NOLOCALE) +pg_regress_clean_files = results/ regression.diffs regression.out tmp_check/ tmp_check_iso/ log/ output_iso/ -pg_regress_check = $(with_temp_install) $(top_builddir)/src/test/regress/pg_regress --inputdir=$(srcdir) --temp-instance=./tmp_check $(TEMP_CONF) --bindir= $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) -pg_regress_installcheck = $(top_builddir)/src/test/regress/pg_regress --inputdir=$(srcdir) --bindir='$(bindir)' $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) +pg_regress_check = \ + $(with_temp_install) \ + $(top_builddir)/src/test/regress/pg_regress \ + --temp-instance=./tmp_check \ + --inputdir=$(srcdir) \ + $(TEMP_CONF) \ + --bindir= $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) +pg_regress_installcheck = \ + $(top_builddir)/src/test/regress/pg_regress \ + --inputdir=$(srcdir) \ + --bindir='$(bindir)' $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) -pg_regress_clean_files = results/ regression.diffs regression.out tmp_check/ log/ - -pg_isolation_regress_check = $(with_temp_install) $(top_builddir)/src/test/isolation/pg_isolation_regress --inputdir=$(srcdir) --temp-instance=./tmp_check $(TEMP_CONF) --bindir= $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) -pg_isolation_regress_installcheck = $(top_builddir)/src/test/isolation/pg_isolation_regress --inputdir=$(srcdir) $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) +pg_isolation_regress_check = $(MKDIR_P) output_iso && \ + $(with_temp_install) \ + $(top_builddir)/src/test/isolation/pg_isolation_regress \ + --temp-instance=./tmp_check_iso \ + --inputdir=$(srcdir) --outputdir=output_iso \ + $(TEMP_CONF) \ + --bindir= $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) +pg_isolation_regress_installcheck = \ + $(top_builddir)/src/test/isolation/pg_isolation_regress \ + --inputdir=$(srcdir) \ + $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS) ########################################################################## # diff --git a/src/test/isolation/.gitignore b/src/test/isolation/.gitignore index 42ee945744..44bcf95854 100644 --- a/src/test/isolation/.gitignore +++ b/src/test/isolation/.gitignore @@ -7,6 +7,5 @@ /specscanner.c # Generated subdirectories -/results/ -/log/ -/tmp_check/ +/output_iso/ +/tmp_check_iso/ diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile index 3d272d5b59..8eb4969e9b 100644 --- a/src/test/isolation/Makefile +++ b/src/test/isolation/Makefile @@ -52,17 +52,17 @@ maintainer-clean: distclean rm -f specparse.c specscanner.c installcheck: all - ./pg_isolation_regress --bindir='$(bindir)' $(EXTRA_REGRESS_OPTS) --inputdir=$(srcdir) --schedule=$(srcdir)/isolation_schedule + $(pg_isolation_regress_installcheck) --schedule=$(srcdir)/isolation_schedule check: all - $(with_temp_install) ./pg_isolation_regress --temp-instance=./tmp_check $(TEMP_CONF) --inputdir=$(srcdir) --bindir= $(EXTRA_REGRESS_OPTS) --schedule=$(srcdir)/isolation_schedule + $(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule # Versions of the check tests that include the prepared_transactions test # It only makes sense to run these if set up to use prepared transactions, # via TEMP_CONFIG for the check case, or via the postgresql.conf for the # installcheck case. installcheck-prepared-txns: all temp-install - ./pg_isolation_regress --bindir='$(bindir)' $(EXTRA_REGRESS_OPTS) --inputdir=$(srcdir) --schedule=$(srcdir)/isolation_schedule prepared-transactions + $(pg_isolation_regress_installcheck) --schedule=$(srcdir)/isolation_schedule prepared-transactions check-prepared-txns: all temp-install - ./pg_isolation_regress --temp-instance=./tmp_check $(TEMP_CONF) $(EXTRA_REGRESS_OPTS) --inputdir=$(srcdir) --schedule=$(srcdir)/isolation_schedule prepared-transactions + $(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions diff --git a/src/test/modules/snapshot_too_old/.gitignore b/src/test/modules/snapshot_too_old/.gitignore index ef3609b7da..5cf29ed6f8 100644 --- a/src/test/modules/snapshot_too_old/.gitignore +++ b/src/test/modules/snapshot_too_old/.gitignore @@ -1 +1 @@ -/isolation_output/ +/output_iso/ diff --git a/src/test/modules/snapshot_too_old/Makefile b/src/test/modules/snapshot_too_old/Makefile index 16339f0366..a72bfad43a 100644 --- a/src/test/modules/snapshot_too_old/Makefile +++ b/src/test/modules/snapshot_too_old/Makefile @@ -1,6 +1,8 @@ # src/test/modules/snapshot_too_old/Makefile -EXTRA_CLEAN = ./isolation_output +# 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) ISOLATIONCHECKS=sto_using_cursor sto_using_select @@ -32,10 +34,8 @@ submake-test_snapshot_too_old: $(MAKE) -C $(top_builddir)/src/test/modules/snapshot_too_old isolationcheck: | submake-isolation submake-test_snapshot_too_old temp-install - $(MKDIR_P) isolation_output $(pg_isolation_regress_check) \ --temp-config $(top_srcdir)/src/test/modules/snapshot_too_old/sto.conf \ - --outputdir=./isolation_output \ $(ISOLATIONCHECKS) isolationcheck-install-force: all | submake-isolation submake-test_snapshot_too_old temp-install -- 2.11.0.22.g8d7a455.dirty
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
