Thread: 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");
Re: pgsql: Move tablespace path re-creation from the makefiles to pg_regres
From
Michael Paquier
Date:
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
Attachment
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
Re: pgsql: Move tablespace path re-creation from the makefiles to pg_regres
From
Michael Paquier
Date:
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