Re: pg_regress cleans up tablespace twice.

Thank you for looking this!

At Fri, 15 May 2020 11:58:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, May 11, 2020 at 05:13:54PM +0900, Kyotaro Horiguchi wrote:
> > Tablespace directory cleanup is not done for all testing
> > targets. Actually it is not done for the tools under bin/ except
> > pg_upgrade.
> 
> Let's first take one problem at a time, as I can see that your patch
> 0002 is modifying a portion of what you added in 0001, and so let's
> try to remove this WIN32 stuff from pg_regress.c.

Yes, 0001 and 0001+0002 are alternatives.  They should be merged if we
are going to fix the pg_upgrade test.  I take this as we go on 0001+0002.

> +sub CleanupTablespaceDirectory
> +{
> +   my $tablespace = 'testtablespace';
> +
> +   rmtree($tablespace) if (-e $tablespace);
> +   mkdir($tablespace);
> +}
> This check should use "-d" and not "-e" as it would be true for a file
> as well.  Also, in pg_regress.c, we remove the existing tablespace

That was intentional so that a file with the name don't stop
testing. Actually pg_regress is checking only for a directory in other
place and it's not that bad since no-one can create a file with that
name while running test.  On the other hand, is there any reason for
refraining from removing if it weren't a directory but a file?

Changed to -d in the attached.

> as well.  Also, in pg_regress.c, we remove the existing tablespace
> test directory in --outputdir, which is "." by default but it can be a
> custom one.  Shouldn't you do the same logic in this new routine?  So
> we should have an optional argument for the output directory that
> defaults to `pwd` if not defined, no?  This means passing down the
> argument only for upgradecheck() in vcregress.pl.

I thought of that but didn't in the patch.  I refrained from doing
that because the output directory is dedicatedly created at the only
place (pg_upgrade test) where the --outputdir is specified. (I think I
tend to do too-much.)

It is easy in perl scripts, but rather complex for makefiles. The
attached is using a perl one-liner to extract outputdir from
EXTRA_REGRESS_OPTS. I don't like that but I didn't come up with better
alternatives.  On the other hand ActivePerl (with default
installation) doesn't seem to know Getopt::Long::GetOptions and
friends.  In the attached vcregress.pl parses --outputdir not using
GetOpt::Long...

>  sub isolationcheck
>  {
>      chdir "../isolation";
> +    CleanupTablespaceDirectory();
>      copy("../../../$Config/isolationtester/isolationtester.exe",
>          "../../../$Config/pg_isolation_regress");
>      my @args = (
> [...]
>      print "============================================================\n";
>      print "Checking $module\n";
> +    CleanupTablespaceDirectory();
>      my @args = (
>          "$topdir/$Config/pg_regress/pg_regress",
>          "--bindir=${topdir}/${Config}/psql",
> I would put that just before the system() calls for consistency with
> the rest.

Right. That's just an mistake. Fixed along with subdircheck.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From db38bd5cdbea950cdeba8bd4745801e9c0a2824c 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] 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.  That is not only ugly also leads to do the clean up
twice at once.  Let's move the cleanup code out of pg_regress into
vcregress.pl, where is more sutable place.

This also fixes a bug that the test for pg_upgrade mistakenly cleans
up the tablespace directory of default tmp-install location, instead
of its own installation directoty.
---
 src/test/regress/GNUmakefile  |  6 ++++--
 src/test/regress/pg_regress.c | 22 ----------------------
 src/tools/msvc/vcregress.pl   | 25 +++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 1a3164065f..815d87904b 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -118,8 +118,10 @@ submake-contrib-spi: | submake-libpgport submake-generated-headers
 
 .PHONY: tablespace-setup
 tablespace-setup:
-    rm -rf ./testtablespace
-    mkdir ./testtablespace
+# extract --outputdir optsion from EXTRA_REGRESS_OPTS
+    $(eval dir := $(shell perl -e 'use Getopt::Long qw(GetOptionsFromString Configure); Configure("pass_through",
"gnu_getopt");($$r, $$x) = GetOptionsFromString($$ENV{"EXTRA_REGRESS_OPTS"}, "outputdir=s" => \$$dir); print ((defined
$$dir? $$dir : ".") . "/testtablespace");'))
 
+    rm -rf $(dir)
+    mkdir $(dir)
 
 
 ##
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 0a98f6e37d..4bada14092 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -114,6 +114,13 @@ sub installcheck_internal
         "--no-locale");
     push(@args, $maxconn) if $maxconn;
     push(@args, @EXTRA_REGRESS_OPTS);
+
+    my $outputdir;
+    foreach (@EXTRA_REGRESS_OPTS)
+    {
+        $outputdir = $1 if (/^ *--outputdir *= *(.+) *$/);
+    }
+    CleanupTablespaceDirectory($outputdir);
     system(@args);
     my $status = $? >> 8;
     exit $status if $status;
@@ -143,6 +150,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;
@@ -169,6 +177,7 @@ sub ecpgcheck
         "--no-locale",
         "--temp-instance=./tmp_chk");
     push(@args, $maxconn) if $maxconn;
+    CleanupTablespaceDirectory();
     system(@args);
     $status = $? >> 8;
     exit $status if $status;
@@ -186,6 +195,7 @@ sub isolationcheck
         "--inputdir=.",
         "--schedule=./isolation_schedule");
     push(@args, $maxconn) if $maxconn;
+    CleanupTablespaceDirectory();
     system(@args);
     my $status = $? >> 8;
     exit $status if $status;
@@ -382,6 +392,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;
@@ -444,6 +455,7 @@ sub subdircheck
         "--bindir=${topdir}/${Config}/psql",
         "--dbname=contrib_regression", @opts, @tests);
     print join(' ', @args), "\n";
+    CleanupTablespaceDirectory();
     system(@args);
     chdir "..";
     return;
@@ -739,6 +751,19 @@ sub InstallTemp
     return;
 }
 
+sub CleanupTablespaceDirectory
+{
+    my ($testdir) = @_;
+
+    $testdir = cwd() if (! defined $testdir);
+
+    # don't bother checking trailing directory separator in $testdir
+    my $tablespace = "$testdir/testtablespace";
+
+    rmtree($tablespace) if (-d $tablespace);
+    mkdir($tablespace);
+}
+
 sub usage
 {
     print STDERR
-- 
2.18.2