Re: pg_regress cleans up tablespace twice.

At Fri, 21 Feb 2020 17:05:07 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Thu, 20 Feb 2020 14:23:22 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> > Removing this code from pg_regress.c makes also sense to me.  Now, the
> > patch breaks "vcregress installcheck" as this is missing to patch
> > installcheck_internal() for the tablespace path creation.  I would
> > also recommend using a full path for the directory location to avoid
> > any potential issues if this code is refactored or moved around, the
> > patch now relying on the current path used.
> 
> Hmm. Right. I confused database directory and tablespace
> directory. Tablespace directory should be provided by the test script,
> even though the database directory is preexisting in installcheck. To
> avoid useless future failure, I would do that that for all
> subcommands, as regress/GNUmakefile does.

Tablespace directory cleanup is not done for all testing
targets. Actually it is not done for the tools under bin/ except
pg_upgrade.

On the other hand, it was done every by pg_regress run for Windows
build.  So I made vcregress.pl do the same with that. Spcecifically to
set up tablespace always before pg_regress is executed.

There is a place where --outputdir is specified for pg_regress,
pg_upgrade/test.sh. It is explained as the follows.

# Send installcheck outputs to a private directory.  This avoids conflict when
# check-world runs pg_upgrade check concurrently with src/test/regress check.
# To retrieve interesting files after a run, use pattern tmp_check/*/*.diffs.
outputdir="$temp_root/regress"
EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"

Where the $temp_root is $(TOP)/src/bin/pg_upgrade/tmp_check/regress.

Thus the current regress/GNUMakefile does break this consideration and
the current vc_regress (of Windows build) does the right thing in the
light of the comment.  Don't we need to avoid cleaning up
"$(TOP)/src/test/regress/tablesapce" in that case? (the second patch
attached)


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From fb351e0b075fbb2e6398aa0991b5a824e3c95e5a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 30 Apr 2020 14:06:51 +0900
Subject: [PATCH 1/2] Move tablespace cleanup out of pg_regress.

Tablespace directory is cleaned-up in regress/GNUmakefile for all
platforms other than Windows. For Windoiws pg_regress does that on
behalf of make, which is ugly. In addition to that, pg_regress does
the clean up twice at once. This patch moves the cleanup code out of
pg_regress into vcregress.pl, which is more sutable place.
---
 src/test/regress/pg_regress.c | 22 ----------------------
 src/tools/msvc/vcregress.pl   | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 38b2b1e8e1..c56bfaf7f5 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -491,28 +491,6 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch
 
     snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);
 
-#ifdef WIN32
-
-    /*
-     * On Windows only, clean out the test tablespace dir, or create it if it
-     * doesn't exist.  On other platforms we expect the Makefile to take care
-     * of that.  (We don't migrate that functionality in here because it'd be
-     * harder to cope with platform-specific issues such as SELinux.)
-     *
-     * XXX it would be better if pg_regress.c had nothing at all to do with
-     * testtablespace, and this were handled by a .BAT file or similar on
-     * Windows.  See pgsql-hackers discussion of 2008-01-18.
-     */
-    if (directory_exists(testtablespace))
-        if (!rmtree(testtablespace, true))
-        {
-            fprintf(stderr, _("\n%s: could not remove test tablespace \"%s\"\n"),
-                    progname, testtablespace);
-            exit(2);
-        }
-    make_directory(testtablespace);
-#endif
-
     /* finally loop on each file and do the replacement */
     for (name = names; *name; name++)
     {
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index f95f7a5c7a..74d37a31de 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -104,6 +104,7 @@ exit 0;
 sub installcheck_internal
 {
     my ($schedule, @EXTRA_REGRESS_OPTS) = @_;
+
     my @args = (
         "../../../$Config/pg_regress/pg_regress",
         "--dlpath=.",
@@ -114,6 +115,7 @@ sub installcheck_internal
         "--no-locale");
     push(@args, $maxconn) if $maxconn;
     push(@args, @EXTRA_REGRESS_OPTS);
+    CleanupTablespaceDirectory();
     system(@args);
     my $status = $? >> 8;
     exit $status if $status;
@@ -143,6 +145,7 @@ sub check
         "--temp-instance=./tmp_check");
     push(@args, $maxconn)     if $maxconn;
     push(@args, $temp_config) if $temp_config;
+    CleanupTablespaceDirectory();
     system(@args);
     my $status = $? >> 8;
     exit $status if $status;
@@ -178,6 +181,7 @@ sub ecpgcheck
 sub isolationcheck
 {
     chdir "../isolation";
+    CleanupTablespaceDirectory();
     copy("../../../$Config/isolationtester/isolationtester.exe",
         "../../../$Config/pg_isolation_regress");
     my @args = (
@@ -382,6 +386,7 @@ sub plcheck
             "$topdir/$Config/pg_regress/pg_regress",
             "--bindir=$topdir/$Config/psql",
             "--dbname=pl_regression", @lang_args, @tests);
+        CleanupTablespaceDirectory();
         system(@args);
         my $status = $? >> 8;
         exit $status if $status;
@@ -439,6 +444,7 @@ sub subdircheck
 
     print "============================================================\n";
     print "Checking $module\n";
+    CleanupTablespaceDirectory();
     my @args = (
         "$topdir/$Config/pg_regress/pg_regress",
         "--bindir=${topdir}/${Config}/psql",
@@ -741,6 +747,14 @@ sub InstallTemp
     return;
 }
 
+sub CleanupTablespaceDirectory
+{
+    my $tablespace = 'testtablespace';
+
+    rmtree($tablespace) if (-e $tablespace);
+    mkdir($tablespace);
+}
+
 sub usage
 {
     print STDERR
-- 
2.18.2

From ca99ec5ff7f23308279098f26ac58a8e927d2646 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Fri, 1 May 2020 09:54:21 +0900
Subject: [PATCH 2/2] Don't setup tablespace directory while testing pg_upgrade

While make check of pg_upgrade, output directory is placed at a
separate place from ordinary tmp_install. However Makefile
reinitializes tablespace directory under the ordinary tmp_install,
which may be being used by concurrent backend make check.  The
dedicate output directory doesn't need initialization so we just avoid
tablespace initialization for the case.
---
 GNUmakefile.in               | 4 ++--
 src/bin/pg_upgrade/test.sh   | 2 +-
 src/test/regress/GNUmakefile | 4 +++-
 src/tools/msvc/vcregress.pl  | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/GNUmakefile.in b/GNUmakefile.in
index ee636e3b50..943878e99b 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -63,8 +63,8 @@ distclean maintainer-clean:
     @rm -rf autom4te.cache/
     rm -f config.cache config.log config.status GNUmakefile
 
-check check-tests installcheck installcheck-parallel installcheck-tests: CHECKPREP_TOP=src/test/regress
-check check-tests installcheck installcheck-parallel installcheck-tests: submake-generated-headers
+check check-tests installcheck installcheck-parallel installcheck-parallel-notspsetup installcheck-tests:
CHECKPREP_TOP=src/test/regress
+check check-tests installcheck installcheck-parallel installcheck-parallel-notspsetup installcheck-tests:
submake-generated-headers
     $(MAKE) -C src/test/regress $@
 
 $(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib src/bin,check)
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 10a28d8133..7be62bd49a 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -166,7 +166,7 @@ createdb "regression$dbname1" || createdb_status=$?
 createdb "regression$dbname2" || createdb_status=$?
 createdb "regression$dbname3" || createdb_status=$?
 
-if "$MAKE" -C "$oldsrc" installcheck-parallel; then
+if "$MAKE" -C "$oldsrc" installcheck-parallel-notspsetup; then
     oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
 
     # before dumping, get rid of objects not existing in later versions
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 1a3164065f..b8681dbd17 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -137,7 +137,9 @@ check-tests: all tablespace-setup | temp-install
 installcheck: all tablespace-setup
     $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_schedule $(EXTRA_TESTS)
 
-installcheck-parallel: all tablespace-setup
+installcheck-parallel: tablespace-setup installcheck-parallel-notspsetup
+
+installcheck-parallel-notspsetup: all
     $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
 
 installcheck-tests: all tablespace-setup
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 74d37a31de..4516db1f38 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -115,7 +115,6 @@ sub installcheck_internal
         "--no-locale");
     push(@args, $maxconn) if $maxconn;
     push(@args, @EXTRA_REGRESS_OPTS);
-    CleanupTablespaceDirectory();
     system(@args);
     my $status = $? >> 8;
     exit $status if $status;
@@ -125,6 +124,7 @@ sub installcheck_internal
 sub installcheck
 {
     my $schedule = shift || 'serial';
+    CleanupTablespaceDirectory();
     installcheck_internal($schedule);
     return;
 }
-- 
2.18.2