On Fri, May 15, 2020 at 05:25:08PM +0900, Kyotaro Horiguchi wrote: > 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.)
So, I have reviewed the patch aimed at removing the cleanup of testtablespace done with WIN32, and finished with the attached to clean up things. I simplified the logic, to not have to parse REGRESS_OPTS for --outputdir (no need for a regex, leaving EXTRA_REGRESS_OPTS alone), and reworked the code so as the tablespace cleanup only happens only where we need to: check, installcheck and upgradecheck. No need for that with contribcheck, modulescheck, plcheck and ecpgcheck. Note that after I changed my patch, this converged with a portion of patch 0002 you have posted here: https://www.postgresql.org/message-id/20200511.171354.514381788845037011.horikyota....@gmail.com Now about 0002, I tend to agree that we should try to do something about pg_upgrade test creating removing and then creating an extra testtablespace path that is not necessary as pg_upgrade test uses its own --outputdir. I have not actually seen this stuff being a problem in practice as the main regression test suite runs first, largely before pg_upgrade test even with parallel runs so they have a low probability of conflict. I'll try to think about a couple of options, one of them I have in mind now being that we could finally switch the upgrade tests to TAP and let test.sh go to the void. This is an independent problem, so let's tackle both issues separately. -- Michael
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index f11a3b9e26..c8d190d248 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -494,28 +494,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 3365ee578c..f4ea1ed9cb 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -123,6 +123,8 @@ sub installcheck_internal
sub installcheck
{
my $schedule = shift || 'serial';
+
+ CleanupTablespaceDirectory();
installcheck_internal($schedule);
return;
}
@@ -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;
@@ -571,7 +574,7 @@ sub upgradecheck
my $outputdir = "$tmp_root/regress";
my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
mkdir "$outputdir" || die $!;
- mkdir "$outputdir/testtablespace" || die $!;
+ CleanupTablespaceDirectory($outputdir);
my $logdir = "$topdir/src/bin/pg_upgrade/log";
rmtree($logdir);
@@ -737,6 +740,16 @@ sub InstallTemp
return;
}
+sub CleanupTablespaceDirectory
+{
+ my $testdir = shift || getcwd();
+
+ my $testtablespace = "$testdir/testtablespace";
+
+ rmtree($testtablespace) if (-d $testtablespace);
+ mkdir($testtablespace);
+}
+
sub usage
{
print STDERR
signature.asc
Description: PGP signature
