Date: Mon, 10 Jan 2022 16:07:48 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>,
	Andrew Dunstan <andrew@dunslane.net>,
	Melanie Plageman <melanieplageman@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Daniel Gustafsson <daniel@yesql.se>
Subject: Re: Adding CI to our tree
Message-ID: <20220110220748.GS14051@telsasoft.com>
References: <20211001222752.wrz7erzh4cajvgp6@alap3.anarazel.de>
 <20211231014652.kgrdk2wytiallix3@alap3.anarazel.de>
 <20220109191649.GL14051@telsasoft.com>
 <20220109195744.mjoue2pr6xtnsquw@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="i/CQJCAqWP/GQJtX"
Content-Disposition: inline
In-Reply-To: <20220109195744.mjoue2pr6xtnsquw@alap3.anarazel.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220110220748.GS14051%40telsasoft.com>
Precedence: bulk


--i/CQJCAqWP/GQJtX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Jan 09, 2022 at 11:57:44AM -0800, Andres Freund wrote:
> On 2022-01-09 13:16:50 -0600, Justin Pryzby wrote:
> > diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
> > index 9a31e0b8795..14fd847ba7f 100644
> > --- a/contrib/test_decoding/Makefile
> > +++ b/contrib/test_decoding/Makefile
> > @@ -10,7 +10,7 @@ ISOLATION = mxact delayed_startup ondisk_startup concurrent_ddl_dml \
> >  	oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
> >  	twophase_snapshot
> >  
> > -REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
> > +REGRESS_OPTS = --temp-config=$(top_srcdir)/contrib/test_decoding/logical.conf
> >  ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
> 
> Not sure why these are part of the diff?

Because otherwise vcregress runs pg_regress --temp-config test1 test2 [...]
..which means test1 gets eaten as the argument to --temp-config

> > diff --git a/src/tools/ci/pg_ci_base.conf b/src/tools/ci/pg_ci_base.conf
> > index d8faa9c26c1..52cdb697a57 100644
> > --- a/src/tools/ci/pg_ci_base.conf
> > +++ b/src/tools/ci/pg_ci_base.conf
> > @@ -12,3 +12,24 @@ log_connections = true
> >  log_disconnections = true
> >  log_line_prefix = '%m [%p][%b] %q[%a][%v:%x] '
> >  log_lock_waits = true
> > +
> > +# test_decoding
> > +wal_level = logical
> > +max_replication_slots = 4
> > +logical_decoding_work_mem = 64kB
> > [ more ]
> 
> This doesn't really seem like a scalable path forward - duplicating
> configuration in more places doesn't seem sane. It seems it'd make more sense
> to teach vcregress.pl to run NO_INSTALLCHECK targets properly? ISTM that
> changing the options passed to pg_regress based on fetchTests() return value
> wouldn't be too hard?

It needs to run the tests with separate instance.  Maybe you're suggesting to
use --temp-instance.

It needs to avoid running on the buildfarm, right ?

-- 
Justin

--i/CQJCAqWP/GQJtX
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-vcregress-ci-test-modules-contrib-with-NO_INSTALLCHE.patch"

From 0818f79b27de42182e26dd9dad991de8258c8238 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 9 Jan 2022 18:25:02 -0600
Subject: [PATCH 1/3] vcregress/ci: test modules/contrib with NO_INSTALLCHECK=1

---
 .cirrus.yml                                |  4 +-
 contrib/pg_stat_statements/Makefile        |  2 +-
 contrib/test_decoding/Makefile             |  2 +-
 src/test/modules/snapshot_too_old/Makefile |  2 +-
 src/test/modules/worker_spi/Makefile       |  2 +-
 src/tools/msvc/vcregress.pl                | 46 +++++++++++++++++++---
 6 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 19b3737fa11..02ea7e67189 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -398,9 +398,9 @@ task:
   test_isolation_script:
     - perl src/tools/msvc/vcregress.pl isolationcheck
   test_modules_script:
-    - perl src/tools/msvc/vcregress.pl modulescheck
+    - perl src/tools/msvc/vcregress.pl modulescheck install
   test_contrib_script:
-    - perl src/tools/msvc/vcregress.pl contribcheck
+    - perl src/tools/msvc/vcregress.pl contribcheck install
   stop_script:
     - tmp_install\bin\pg_ctl.exe stop -D tmp_check/db -l tmp_check/postmaster.log
   test_ssl_script:
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 7fabd96f38d..d732e1ade73 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -15,7 +15,7 @@ PGFILEDESC = "pg_stat_statements - execution statistics of SQL statements"
 
 LDFLAGS_SL += $(filter -lm, $(LIBS))
 
-REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
+REGRESS_OPTS = --temp-config=$(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
 REGRESS = pg_stat_statements oldextversions
 # Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
 # which typical installcheck users do not have (e.g. buildfarm clients).
diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index 9a31e0b8795..14fd847ba7f 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -10,7 +10,7 @@ ISOLATION = mxact delayed_startup ondisk_startup concurrent_ddl_dml \
 	oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
 	twophase_snapshot
 
-REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
+REGRESS_OPTS = --temp-config=$(top_srcdir)/contrib/test_decoding/logical.conf
 ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
 
 # Disabled because these tests require "wal_level=logical", which
diff --git a/src/test/modules/snapshot_too_old/Makefile b/src/test/modules/snapshot_too_old/Makefile
index dfb4537f63c..752a0039fdc 100644
--- a/src/test/modules/snapshot_too_old/Makefile
+++ b/src/test/modules/snapshot_too_old/Makefile
@@ -5,7 +5,7 @@
 EXTRA_CLEAN = $(pg_regress_clean_files)
 
 ISOLATION = sto_using_cursor sto_using_select sto_using_hash_index
-ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/snapshot_too_old/sto.conf
+ISOLATION_OPTS = --temp-config=$(top_srcdir)/src/test/modules/snapshot_too_old/sto.conf
 
 # Disabled because these tests require "old_snapshot_threshold" >= 0, which
 # typical installcheck users do not have (e.g. buildfarm clients).
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index cbf9b2e37fd..d9f7d9bab6d 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -9,7 +9,7 @@ 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
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/worker_spi/dynamic.conf
 
 # Disable installcheck to ensure we cover dynamic bgworkers.
 NO_INSTALLCHECK = 1
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 8f3e3fa937b..c751a53e5d4 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -443,6 +443,7 @@ sub plcheck
 sub subdircheck
 {
 	my $module = shift;
+	my $installcheck = shift || 1;
 
 	if (   !-d "$module/sql"
 		|| !-d "$module/expected"
@@ -452,7 +453,7 @@ sub subdircheck
 	}
 
 	chdir $module;
-	my @tests = fetchTests();
+	my @tests = fetchTests($installcheck);
 
 	# Leave if no tests are listed in the module.
 	if (scalar @tests == 0)
@@ -462,6 +463,7 @@ sub subdircheck
 	}
 
 	my @opts = fetchRegressOpts();
+	push @opts, "--temp-instance=tmp_check" if $installcheck == -1;
 
 	# Special processing for python transform modules, see their respective
 	# Makefiles for more details regarding Python-version specific
@@ -489,7 +491,7 @@ sub subdircheck
 	print "Checking $module\n";
 	my @args = (
 		"$topdir/$Config/pg_regress/pg_regress",
-		"--bindir=${topdir}/${Config}/psql",
+		"--bindir=$tmp_installdir/bin",
 		"--dbname=contrib_regression", @opts, @tests);
 	print join(' ', @args), "\n";
 	system(@args);
@@ -499,6 +501,8 @@ sub subdircheck
 
 sub contribcheck
 {
+	my $mode = shift || '';
+
 	chdir "../../../contrib";
 	my $mstat = 0;
 	foreach my $module (glob("*"))
@@ -516,12 +520,25 @@ sub contribcheck
 		my $status = $? >> 8;
 		$mstat ||= $status;
 	}
+
+	# As above, but creates new DB instance for each module.  For CI.
+	if ($mode eq "install")
+	{
+		foreach my $module (glob("*"))
+		{
+			subdircheck("$module", -1);
+			$mstat ||= $? >> 8;
+		}
+	}
+
 	exit $mstat if $mstat;
 	return;
 }
 
 sub modulescheck
 {
+	my $mode = shift || '';
+
 	chdir "../../../src/test/modules";
 	my $mstat = 0;
 	foreach my $module (glob("*"))
@@ -530,6 +547,17 @@ sub modulescheck
 		my $status = $? >> 8;
 		$mstat ||= $status;
 	}
+
+	# As above, but creates new DB instance for each module.  For CI.
+	if ($mode eq "install")
+	{
+		foreach my $module (glob("*"))
+		{
+			subdircheck("$module", -1);
+			$mstat ||= $? >> 8;
+		}
+	}
+
 	exit $mstat if $mstat;
 	return;
 }
@@ -700,6 +728,7 @@ sub fetchRegressOpts
 		# option starting with "--".
 		@opts = grep { !/\$\(/ && /^--/ }
 		  map { (my $x = $_) =~ s/\Q$(top_builddir)\E/\"$topdir\"/; $x; }
+		  map { (my $x = $_) =~ s/\Q$(top_srcdir)\E/\"$topdir\"/; $x; }
 		  split(/\s+/, $1);
 	}
 	if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
@@ -726,14 +755,19 @@ sub fetchTests
 	my $m = <$handle>;
 	close($handle);
 	my $t = "";
+	my $installcheck = shift || 1;
 
 	$m =~ s{\\\r?\n}{}g;
 
-	# A module specifying NO_INSTALLCHECK does not support installcheck,
-	# so bypass its run by returning an empty set of tests.
 	if ($m =~ /^\s*NO_INSTALLCHECK\s*=\s*\S+/m)
 	{
-		return ();
+		# Skip modules marked installcheck unless running installcheck tests.
+		return () if $installcheck == 1;
+	}
+	else
+	{
+		# Skip modules not marked installcheck if running installcheck tests.
+		return () if $installcheck == -1;
 	}
 
 	if ($m =~ /^REGRESS\s*=\s*(.*)$/gm)
@@ -799,6 +833,8 @@ sub usage
 	  "\nOptions for <arg>: (used by check and installcheck)\n",
 	  "  serial         serial mode\n",
 	  "  parallel       parallel mode\n",
+	  "\nOptions for <arg>: (used by contribcheck and modulescheck)\n",
+	  "  install        also run tests which require a new instance\n",
 	  "\nOption for <arg>: for taptest\n",
 	  "  TEST_DIR       (required) directory where tests reside\n";
 	exit(1);
-- 
2.17.1


--i/CQJCAqWP/GQJtX
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0002-CI-run-initdb-with-no-sync-for-windows.patch"

From 2a3bf17c13eefd6516f744e2e314669e988f9422 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 9 Jan 2022 22:54:32 -0600
Subject: [PATCH 2/3] CI: run initdb with --no-sync for windows

---
 .cirrus.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 02ea7e67189..a0d733bb594 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -390,7 +390,7 @@ task:
     - perl src/tools/msvc/vcregress.pl check parallel
   startcreate_script:
     # paths to binaries need backslashes
-    - tmp_install\bin\pg_ctl.exe initdb -D tmp_check/db -l tmp_check/initdb.log
+    - tmp_install\bin\pg_ctl.exe initdb -D tmp_check/db -l tmp_check/initdb.log --options=--no-sync
     - echo include '%TEMP_CONFIG%' >> tmp_check/db/postgresql.conf
     - tmp_install\bin\pg_ctl.exe start -D tmp_check/db -l tmp_check/postmaster.log
   test_pl_script:
-- 
2.17.1


--i/CQJCAqWP/GQJtX
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0003-vcregress-style.patch"

From 91e134926ef60f63f191b2664f09fef8b4a9ffeb Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 9 Jan 2022 23:05:18 -0600
Subject: [PATCH 3/3] vcregress: style

---
 src/tools/msvc/vcregress.pl | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index c751a53e5d4..62a2667cb4f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -566,7 +566,6 @@ sub recoverycheck
 {
 	InstallTemp();
 
-	my $mstat  = 0;
 	my $dir    = "$topdir/src/test/recovery";
 	my $status = tap_check($dir);
 	exit $status if $status;
@@ -746,7 +745,6 @@ sub fetchRegressOpts
 # list is returned if the module does not need to run anything.
 sub fetchTests
 {
-
 	my $handle;
 	open($handle, '<', "GNUmakefile")
 	  || open($handle, '<', "Makefile")
-- 
2.17.1


--i/CQJCAqWP/GQJtX--