Обсуждение: Re: pgsql: Move tablespace path re-creation from the makefiles to pg_regres

Michael Paquier <michael@paquier.xyz> writes:
> Move tablespace path re-creation from the makefiles to pg_regress

So this didn't seem like a problem at the time, but while building
beta1 tarballs I discovered that it leaves behind "testtablespace"
subdirectories in various places where they aren't cleaned by
"make distclean", resulting in scary noise in my diff against the
tarballs:

Only in /home/postgres/pgsql/contrib/dblink: testtablespace
Only in /home/postgres/pgsql/contrib/file_fdw: testtablespace
Only in /home/postgres/pgsql/src/pl/plpgsql/src: testtablespace

This appears to be because pg_regress.c will now create the
tablespace directory in any directory that has an "input"
subdirectory (and that randomness is because somebody saw
fit to drop the code into convert_sourcefiles_in(), where
it surely has no business being, not least because that
means it's run twice).

(BTW, the reason we don't see git complaining about this seems
to be that it doesn't complain about empty subdirectories.)

I think what we want to do is have this code invoked only in
test directories that explicitly ask for it, say with a new
"--make-testtablespace" switch for pg_regress.

            regards, tom lane

I wrote:
> I think what we want to do is have this code invoked only in
> test directories that explicitly ask for it, say with a new
> "--make-testtablespace" switch for pg_regress.

Say, as attached.  (Windows part is untested.)

            regards, tom lane

diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 5dc4bbcb00..fe6e0c98aa 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -119,7 +119,8 @@ submake-contrib-spi: | submake-libpgport submake-generated-headers
 ## Run tests
 ##

-REGRESS_OPTS = --dlpath=. --max-concurrent-tests=20 $(EXTRA_REGRESS_OPTS)
+REGRESS_OPTS = --dlpath=. --max-concurrent-tests=20 --make-testtablespace-dir \
+    $(EXTRA_REGRESS_OPTS)

 check: all
     $(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index b7d80bd9bb..5918e8b412 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -504,25 +504,9 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch
     if (!directory_exists(outdir_sub))
         make_directory(outdir_sub);

+    /* We might need to replace @testtablespace@ */
     snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);

-    /*
-     * Clean out the test tablespace dir, or create it if it doesn't exist. On
-     * Windows, doing this cleanup here makes possible to run the regression
-     * tests as a Windows administrative user account with the restricted
-     * token obtained when starting pg_regress.
-     */
-    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);
-
     /* finally loop on each file and do the replacement */
     for (name = names; *name; name++)
     {
@@ -601,6 +585,32 @@ convert_sourcefiles(void)
     convert_sourcefiles_in("output", outputdir, "expected", "out");
 }

+/*
+ * Clean out the test tablespace dir, or create it if it doesn't exist.
+ *
+ * On Windows, doing this cleanup here makes it possible to run the
+ * regression tests under a Windows administrative user account with the
+ * restricted token obtained when starting pg_regress.
+ */
+static void
+prepare_testtablespace_dir(void)
+{
+    char        testtablespace[MAXPGPATH];
+
+    snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);
+
+    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);
+}
+
 /*
  * Scan resultmap file to find which platform-specific expected files to use.
  *
@@ -2062,6 +2072,7 @@ help(void)
     printf(_("                                (default is 0, meaning unlimited)\n"));
     printf(_("      --max-concurrent-tests=N  maximum number of concurrent tests in schedule\n"));
     printf(_("                                (default is 0, meaning unlimited)\n"));
+    printf(_("      --make-testtablespace-dir create testtablespace directory\n"));
     printf(_("      --outputdir=DIR           place output files in DIR (default \".\")\n"));
     printf(_("      --schedule=FILE           use test ordering schedule from FILE\n"));
     printf(_("                                (can be used multiple times to concatenate)\n"));
@@ -2116,10 +2127,12 @@ regression_main(int argc, char *argv[],
         {"load-extension", required_argument, NULL, 22},
         {"config-auth", required_argument, NULL, 24},
         {"max-concurrent-tests", required_argument, NULL, 25},
+        {"make-testtablespace-dir", no_argument, NULL, 26},
         {NULL, 0, NULL, 0}
     };

     bool        use_unix_sockets;
+    bool        make_testtablespace_dir = false;
     _stringlist *sl;
     int            c;
     int            i;
@@ -2245,6 +2258,9 @@ regression_main(int argc, char *argv[],
             case 25:
                 max_concurrent_tests = atoi(optarg);
                 break;
+            case 26:
+                make_testtablespace_dir = true;
+                break;
             default:
                 /* getopt_long already emitted a complaint */
                 fprintf(stderr, _("\nTry \"%s -h\" for more information.\n"),
@@ -2297,6 +2313,9 @@ regression_main(int argc, char *argv[],
     unlimit_core_size();
 #endif

+    if (make_testtablespace_dir)
+        prepare_testtablespace_dir();
+
     if (temp_instance)
     {
         FILE       *pg_conf;
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 1852c34109..35e8f67f01 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -118,6 +118,7 @@ sub installcheck_internal
         "--bindir=../../../$Config/psql",
         "--schedule=${schedule}_schedule",
         "--max-concurrent-tests=20",
+        "--make-testtablespace-dir",
         "--encoding=SQL_ASCII",
         "--no-locale");
     push(@args, $maxconn) if $maxconn;
@@ -152,6 +153,7 @@ sub check
         "--bindir=",
         "--schedule=${schedule}_schedule",
         "--max-concurrent-tests=20",
+        "--make-testtablespace-dir",
         "--encoding=SQL_ASCII",
         "--no-locale",
         "--temp-instance=./tmp_check");

On Mon, May 17, 2021 at 05:51:54PM -0400, Tom Lane wrote:
> I wrote:
>> I think what we want to do is have this code invoked only in
>> test directories that explicitly ask for it, say with a new
>> "--make-testtablespace" switch for pg_regress.
>
> Say, as attached.  (Windows part is untested.)

Thanks.  I was going to produce something this morning, but you have
been faster than me.

One thing that's changing in this patch is that testtablespace would
be created even if the input directory does not exist when using
--make-testtablespace-dir.  I would have kept the creation of the
tablespace path within convert_sourcefiles_in() for this reason.
Worth noting that snprintf() is used twice instead of once to build
the tablespace path string.  The Windows part works correctly.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, May 17, 2021 at 05:51:54PM -0400, Tom Lane wrote:
>> Say, as attached.  (Windows part is untested.)

> One thing that's changing in this patch is that testtablespace would
> be created even if the input directory does not exist when using
> --make-testtablespace-dir.

Yeah, I do not see a reason for there to be a connection.  It's not
pg_regress' job to decide whether the testtablespace directory is
needed or not.

> Worth noting that snprintf() is used twice instead of once to build
> the tablespace path string.

Yeah.  I considered making a global variable so that there'd be
just one instance of that, but didn't think it amounted to less
mess in the end.

> The Windows part works correctly.

Thanks for testing!  I'll push this after the release is tagged.

            regards, tom lane

On Mon, May 17, 2021 at 08:57:55PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> One thing that's changing in this patch is that testtablespace would
>> be created even if the input directory does not exist when using
>> --make-testtablespace-dir.
>
> Yeah, I do not see a reason for there to be a connection.  It's not
> pg_regress' job to decide whether the testtablespace directory is
> needed or not.

Fine by me.  I don't think that's a big deal either way.

>> Worth noting that snprintf() is used twice instead of once to build
>> the tablespace path string.
>
> Yeah.  I considered making a global variable so that there'd be
> just one instance of that, but didn't think it amounted to less
> mess in the end.

No problem from me here either.

>> The Windows part works correctly.
>
> Thanks for testing!  I'll push this after the release is tagged.

Thanks!
--
Michael