pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Remove reset of testtablespace from pg_regress on Windows
5+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2020-07-10 61be85afab Revert "Remove reset of testtablespace from pg_regress on Windows"
* pgsql: Remove reset of testtablespace from pg_regress on Windows
@ 2020-06-18 01:40 Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2020-06-18 01:40 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Remove reset of testtablespace from pg_regress on Windows
testtablespace is an extra path used as tablespace location in the main
regression test suite, computed from --outputdir as defined by the
caller of pg_regress (current directory if undefined).
This special handling was introduced as of f10589e to be specific to
MSVC, as we let pg_regress' Makefile handle this cleanup in other
environments. This moves the cleanup to the MSVC script running
regression tests instead where needed: check, installcheck and
upgradecheck. I have also checked this patch on MSVC with repeated runs
of each target.
Author: Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20200219.142519.437573253063431435.horikyota.ntt@gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/2b2a070d98b2f2c7ecc031e582cfefa400316ce3
Modified Files
--------------
src/test/regress/pg_regress.c | 22 ----------------------
src/tools/msvc/vcregress.pl | 17 +++++++++++++++--
2 files changed, 15 insertions(+), 24 deletions(-)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Remove reset of testtablespace from pg_regress on Windows
@ 2020-07-09 23:28 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Dunstan @ 2020-07-09 23:28 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; pgsql-committers@lists.postgresql.org
On 6/17/20 9:40 PM, Michael Paquier wrote:
> Remove reset of testtablespace from pg_regress on Windows
>
>
>
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.
I assume the testing referred to above was not as a privileged user. I
think this should be reverted.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Remove reset of testtablespace from pg_regress on Windows
@ 2020-07-10 01:02 Michael Paquier <michael@paquier.xyz>
parent: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2020-07-10 01:02 UTC (permalink / raw)
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; +Cc: pgsql-committers@lists.postgresql.org
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.
>
> 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 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.
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 obtained
+ * when starting pg_regress. On other platforms we expect the Makefile
+ * to take care of that.
+ */
What do you think?
--
Michael
Attachments:
regress-win32-tbspace.patch (text/x-diff, 2.3K) download
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, 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 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 = 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 = shift || 'serial';
-
- CleanupTablespaceDirectory();
installcheck_internal($schedule);
return;
}
@@ -145,7 +143,6 @@ 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;
@@ -573,8 +570,8 @@ sub upgradecheck
$ENV{PGDATA} = "$data.old";
my $outputdir = "$tmp_root/regress";
my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
- mkdir "$outputdir" || die $!;
- CleanupTablespaceDirectory($outputdir);
+ mkdir "$outputdir" || die $!;
+ mkdir "$outputdir/testtablespace" || die $!;
my $logdir = "$topdir/src/bin/pg_upgrade/log";
rmtree($logdir);
@@ -740,16 +737,6 @@ sub InstallTemp
return;
}
-sub CleanupTablespaceDirectory
-{
- my $testdir = shift || getcwd();
-
- my $testtablespace = "$testdir/testtablespace";
-
- rmtree($testtablespace) if (-d $testtablespace);
- mkdir($testtablespace);
-}
-
sub usage
{
print STDERR
* Re: pgsql: Remove reset of testtablespace from pg_regress on Windows
@ 2020-07-10 02:21 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Dunstan @ 2020-07-10 02:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-committers@lists.postgresql.org
On 7/9/20 9:02 PM, Michael Paquier wrote:
> 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.
>>
>> 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 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.
>
> 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 obtained
> + * when starting pg_regress. On other platforms we expect the Makefile
> + * to take care of that.
> + */
>
> What do you think?
I certainly agree we should document more clearly why it's there, to
help forestall anyone else who comes along and thinks it would just be
neater to remove it. so +1.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Remove reset of testtablespace from pg_regress on Windows
@ 2020-07-10 08:20 Michael Paquier <michael@paquier.xyz>
parent: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2020-07-10 08:20 UTC (permalink / raw)
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; +Cc: pgsql-committers@lists.postgresql.org
On Thu, Jul 09, 2020 at 10:21:57PM -0400, Andrew Dunstan wrote:
> I certainly agree we should document more clearly why it's there, to
> help forestall anyone else who comes along and thinks it would just be
> neater to remove it. so +1.
Okay. Done, then.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2020-07-10 08:20 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-06-18 01:40 pgsql: Remove reset of testtablespace from pg_regress on Windows Michael Paquier <michael@paquier.xyz>
2020-07-09 23:28 ` Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2020-07-10 01:02 ` Michael Paquier <michael@paquier.xyz>
2020-07-10 02:21 ` Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2020-07-10 08:20 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox