Date: Fri, 10 Jul 2020 10:02:02 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Remove reset of testtablespace from pg_regress on Windows
Message-ID: <20200710010202.GD11153@paquier.xyz>
References: <E1jljXu-0007fp-Ct@gemulon.postgresql.org>
 <6d9eee97-54c8-e14a-48f7-3194e712f54f@2ndQuadrant.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="0H629O+sVkh21xTi"
Content-Disposition: inline
In-Reply-To: <6d9eee97-54c8-e14a-48f7-3194e712f54f@2ndQuadrant.com>
Precedence: bulk


--0H629O+sVkh21xTi
Content-Type: multipart/mixed; boundary="lc9FT7cWel8HagAv"
Content-Disposition: inline


--lc9FT7cWel8HagAv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Jul 09, 2020 at 07:28:02PM -0400, Andrew Dunstan wrote:
> This patch has carefully removed the ability to run the regression tests
> as a Windows administrative user, as I just discovered. This was the
> whole point of commit ce5d3424d6.
>=20
> I assume the testing referred to above was not as a privileged user. I
> think this should be reverted.

Thanks Andrew.  This was discussed on the original thread and what I
wanted to do a rvert if you look at its newest history:
https://www.postgresql.org/message-id/20200623014036.GF50978@paquier.xyz
And then, the thread just stalled..  So I was not sure if something
was actually wanted or not.

Now, I don't think that just a simple revert is the best answer we can
provide.  Just look at this comment in pg_regress.c that does not give
a hint that we actually should not remove this code:
-    * 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 ca=
re
-    * 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.

So instead I would like to propose the attached, reworking this
comment as follows (basically a revert, except for this comment):
+   /*
+    * On Windows only, clean out the test tablespace dir, or create it if =
it
+    * doesn't exist so as it is possible to run the regression tests as a
+    * Windows administrative user account with the restricted token obtain=
ed
+    * when starting pg_regress.  On other platforms we expect the Makefile
+    * to take care of that.
+    */

What do you think?
--
Michael

--lc9FT7cWel8HagAv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="regress-win32-tbspace.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index c8d190d248..8bfc336ec2 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -494,6 +494,25 @@ convert_sourcefiles_in(const char *source_subdir, cons=
t char *dest_dir, const ch
=20
 	snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);
=20
+#ifdef WIN32
+
+	/*
+	 * On Windows only, clean out the test tablespace dir, or create it if it
+	 * doesn't exist so as it is possible to run the regression tests as a
+	 * Windows administrative user account with the restricted token obtained
+	 * when starting pg_regress.  On other platforms we expect the Makefile
+	 * to take care of that.
+	 */
+	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 =3D names; *name; name++)
 	{
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index d6763ad4ac..3365ee578c 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -123,8 +123,6 @@ sub installcheck_internal
 sub installcheck
 {
 	my $schedule =3D shift || 'serial';
-
-	CleanupTablespaceDirectory();
 	installcheck_internal($schedule);
 	return;
 }
@@ -145,7 +143,6 @@ sub check
 		"--temp-instance=3D./tmp_check");
 	push(@args, $maxconn)     if $maxconn;
 	push(@args, $temp_config) if $temp_config;
-	CleanupTablespaceDirectory();
 	system(@args);
 	my $status =3D $? >> 8;
 	exit $status if $status;
@@ -573,8 +570,8 @@ sub upgradecheck
 	$ENV{PGDATA} =3D "$data.old";
 	my $outputdir          =3D "$tmp_root/regress";
 	my @EXTRA_REGRESS_OPTS =3D ("--outputdir=3D$outputdir");
-	mkdir "$outputdir" || die $!;
-	CleanupTablespaceDirectory($outputdir);
+	mkdir "$outputdir"                || die $!;
+	mkdir "$outputdir/testtablespace" || die $!;
=20
 	my $logdir =3D "$topdir/src/bin/pg_upgrade/log";
 	rmtree($logdir);
@@ -740,16 +737,6 @@ sub InstallTemp
 	return;
 }
=20
-sub CleanupTablespaceDirectory
-{
-	my $testdir =3D shift || getcwd();
-
-	my $testtablespace =3D "$testdir/testtablespace";
-
-	rmtree($testtablespace) if (-d $testtablespace);
-	mkdir($testtablespace);
-}
-
 sub usage
 {
 	print STDERR

--lc9FT7cWel8HagAv--

--0H629O+sVkh21xTi
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl8HvgoACgkQnvQgOdby
QH0JxxAAiJh5r1oV9lLcUluCrkhFsRs+PfViKFdxZv5VqLY0X5XWnAHffYvsq1J0
e322Fb4kq/n3dIvhUNK8hHoIPVwtP9Jl25QNRWcCB49JeUZq8ijCBgjH5mOXa/Oe
2WbmELSFIR+22xRdxg+o8BFGrv17tNPdnBjr5YzJPyx1J9hPbG7Z8ScLvOk39zx4
Rj32n24mMbKeIlH87jnC3WRSKeDoKoMrAwhnnbrDrlonbXWljU8bR6pRxhyHXqT2
mBiFAfPF4aqX1oSq+lH8JvvvUtmX+qymHKhJDQBG6h3Xs+a3XPkhI1k75DZhhp0W
mrBwLunkwDWbRZDRc1xxVZGT1YHlU30r2c4+SJ7YlRJM2uEl6zlasF37LDDJNU5g
LFgqsUK7y0/FMa4heKPI47yAngbmaDCDrri1GZzN2bLgc3Mnn6pARaAhnAfXClX8
WR74vNcTLZe36koLUhRUiEW4qzR2MHlMJNhdcEPKOlwjvpopbcfwO3RCcUVyqlhS
GlmEnBofcUEddI7BlxSevyWcLKzEsb1dCBa3uf7gA/P5CQllktu2Xr9uFBBDCK6L
rt/yIkBd5pmA17j72i7M6d5cX2JgbNzd6Y2/UR92jAh3OXwNIyvUFt4dhh5BsjDp
uyT6+9fuX9XB4qON+ywNujb9/A0UYNk8A15Hm2yQKhIX3V+OaG8=
=yd/o
-----END PGP SIGNATURE-----

--0H629O+sVkh21xTi--