Обсуждение: pg_regress cleans up tablespace twice.
Hello. I saw a failure of vcregress check with the following message several times, on a machine under a heavy load and maybe with realtime virus scanning. > pg_regress: could not create directory ".../testtablespace": Permission denied. I found that pg_regress repeats the sequence rmtree(tablespace)->make_directory(tablespace) twice under initialize_environment. So it should be THE DELETE_PENDING. It is because the code is in convert_sourcefiles_in, which is called succssively twice in convert_sourcefiles. But in the first place it comes from [1] and the comment says: > * 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. Is there any reason not to do that in vcregress.pl? I think the commands other than 'check' don't needs this. [1] https://www.postgresql.org/message-id/11718.1200684807%40sss.pgh.pa.us regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 9a4e52bc7b..ae2bbba541 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -490,28 +490,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 82dca29a61..d20d700d15 100644 --- a/src/tools/msvc/vcregress.pl +++ b/src/tools/msvc/vcregress.pl @@ -128,8 +128,15 @@ sub installcheck sub check { my $schedule = shift || 'parallel'; + my $tablespace = 'testtablespace'; + InstallTemp(); chdir "${topdir}/src/test/regress"; + + # Tablespace setup + rmtree($tablespace) if (-e $tablespace); + mkdir($tablespace); + my @args = ( "../../../$Config/pg_regress/pg_regress", "--dlpath=.",
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> But in the first place it comes from [1] and the comment says:
>> * 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.
> Is there any reason not to do that in vcregress.pl? I think the
> commands other than 'check' don't needs this.
I think the existing coding dates from before we had a Perl driver for
this, or else we had it but there were other less-functional ways to
replace "make check" on Windows. +1 for taking the code out of
pg_regress.c --- but I'm not in a position to say whether the other
part of your patch is sufficient.
regards, tom lane
On Wed, Feb 19, 2020 at 04:06:33PM -0500, Tom Lane wrote: > I think the existing coding dates from before we had a Perl driver for > this, or else we had it but there were other less-functional ways to > replace "make check" on Windows. +1 for taking the code out of > pg_regress.c --- but I'm not in a position to say whether the other > part of your patch is sufficient. Removing this code from pg_regress.c makes also sense to me. Now, the patch breaks "vcregress installcheck" as this is missing to patch installcheck_internal() for the tablespace path creation. I would also recommend using a full path for the directory location to avoid any potential issues if this code is refactored or moved around, the patch now relying on the current path used. -- Michael
Вложения
At Thu, 20 Feb 2020 14:23:22 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Wed, Feb 19, 2020 at 04:06:33PM -0500, Tom Lane wrote: > > I think the existing coding dates from before we had a Perl driver for > > this, or else we had it but there were other less-functional ways to > > replace "make check" on Windows. +1 for taking the code out of > > pg_regress.c --- but I'm not in a position to say whether the other > > part of your patch is sufficient. > > Removing this code from pg_regress.c makes also sense to me. Now, the > patch breaks "vcregress installcheck" as this is missing to patch > installcheck_internal() for the tablespace path creation. I would > also recommend using a full path for the directory location to avoid > any potential issues if this code is refactored or moved around, the > patch now relying on the current path used. Hmm. Right. I confused database directory and tablespace directory. Tablespace directory should be provided by the test script, even though the database directory is preexisting in installcheck. To avoid useless future failure, I would do that that for all subcommands, as regress/GNUmakefile does. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Fri, 21 Feb 2020 17:05:07 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> At Thu, 20 Feb 2020 14:23:22 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> > Removing this code from pg_regress.c makes also sense to me. Now, the
> > patch breaks "vcregress installcheck" as this is missing to patch
> > installcheck_internal() for the tablespace path creation. I would
> > also recommend using a full path for the directory location to avoid
> > any potential issues if this code is refactored or moved around, the
> > patch now relying on the current path used.
>
> Hmm. Right. I confused database directory and tablespace
> directory. Tablespace directory should be provided by the test script,
> even though the database directory is preexisting in installcheck. To
> avoid useless future failure, I would do that that for all
> subcommands, as regress/GNUmakefile does.
Tablespace directory cleanup is not done for all testing
targets. Actually it is not done for the tools under bin/ except
pg_upgrade.
On the other hand, it was done every by pg_regress run for Windows
build. So I made vcregress.pl do the same with that. Spcecifically to
set up tablespace always before pg_regress is executed.
There is a place where --outputdir is specified for pg_regress,
pg_upgrade/test.sh. It is explained as the follows.
# Send installcheck outputs to a private directory. This avoids conflict when
# check-world runs pg_upgrade check concurrently with src/test/regress check.
# To retrieve interesting files after a run, use pattern tmp_check/*/*.diffs.
outputdir="$temp_root/regress"
EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"
Where the $temp_root is $(TOP)/src/bin/pg_upgrade/tmp_check/regress.
Thus the current regress/GNUMakefile does break this consideration and
the current vc_regress (of Windows build) does the right thing in the
light of the comment. Don't we need to avoid cleaning up
"$(TOP)/src/test/regress/tablesapce" in that case? (the second patch
attached)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From fb351e0b075fbb2e6398aa0991b5a824e3c95e5a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 30 Apr 2020 14:06:51 +0900
Subject: [PATCH 1/2] Move tablespace cleanup out of pg_regress.
Tablespace directory is cleaned-up in regress/GNUmakefile for all
platforms other than Windows. For Windoiws pg_regress does that on
behalf of make, which is ugly. In addition to that, pg_regress does
the clean up twice at once. This patch moves the cleanup code out of
pg_regress into vcregress.pl, which is more sutable place.
---
src/test/regress/pg_regress.c | 22 ----------------------
src/tools/msvc/vcregress.pl | 14 ++++++++++++++
2 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 38b2b1e8e1..c56bfaf7f5 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -491,28 +491,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 f95f7a5c7a..74d37a31de 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -104,6 +104,7 @@ exit 0;
sub installcheck_internal
{
my ($schedule, @EXTRA_REGRESS_OPTS) = @_;
+
my @args = (
"../../../$Config/pg_regress/pg_regress",
"--dlpath=.",
@@ -114,6 +115,7 @@ sub installcheck_internal
"--no-locale");
push(@args, $maxconn) if $maxconn;
push(@args, @EXTRA_REGRESS_OPTS);
+ CleanupTablespaceDirectory();
system(@args);
my $status = $? >> 8;
exit $status if $status;
@@ -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;
@@ -178,6 +181,7 @@ sub ecpgcheck
sub isolationcheck
{
chdir "../isolation";
+ CleanupTablespaceDirectory();
copy("../../../$Config/isolationtester/isolationtester.exe",
"../../../$Config/pg_isolation_regress");
my @args = (
@@ -382,6 +386,7 @@ sub plcheck
"$topdir/$Config/pg_regress/pg_regress",
"--bindir=$topdir/$Config/psql",
"--dbname=pl_regression", @lang_args, @tests);
+ CleanupTablespaceDirectory();
system(@args);
my $status = $? >> 8;
exit $status if $status;
@@ -439,6 +444,7 @@ sub subdircheck
print "============================================================\n";
print "Checking $module\n";
+ CleanupTablespaceDirectory();
my @args = (
"$topdir/$Config/pg_regress/pg_regress",
"--bindir=${topdir}/${Config}/psql",
@@ -741,6 +747,14 @@ sub InstallTemp
return;
}
+sub CleanupTablespaceDirectory
+{
+ my $tablespace = 'testtablespace';
+
+ rmtree($tablespace) if (-e $tablespace);
+ mkdir($tablespace);
+}
+
sub usage
{
print STDERR
--
2.18.2
From ca99ec5ff7f23308279098f26ac58a8e927d2646 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Fri, 1 May 2020 09:54:21 +0900
Subject: [PATCH 2/2] Don't setup tablespace directory while testing pg_upgrade
While make check of pg_upgrade, output directory is placed at a
separate place from ordinary tmp_install. However Makefile
reinitializes tablespace directory under the ordinary tmp_install,
which may be being used by concurrent backend make check. The
dedicate output directory doesn't need initialization so we just avoid
tablespace initialization for the case.
---
GNUmakefile.in | 4 ++--
src/bin/pg_upgrade/test.sh | 2 +-
src/test/regress/GNUmakefile | 4 +++-
src/tools/msvc/vcregress.pl | 2 +-
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/GNUmakefile.in b/GNUmakefile.in
index ee636e3b50..943878e99b 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -63,8 +63,8 @@ distclean maintainer-clean:
@rm -rf autom4te.cache/
rm -f config.cache config.log config.status GNUmakefile
-check check-tests installcheck installcheck-parallel installcheck-tests: CHECKPREP_TOP=src/test/regress
-check check-tests installcheck installcheck-parallel installcheck-tests: submake-generated-headers
+check check-tests installcheck installcheck-parallel installcheck-parallel-notspsetup installcheck-tests:
CHECKPREP_TOP=src/test/regress
+check check-tests installcheck installcheck-parallel installcheck-parallel-notspsetup installcheck-tests:
submake-generated-headers
$(MAKE) -C src/test/regress $@
$(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib src/bin,check)
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 10a28d8133..7be62bd49a 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -166,7 +166,7 @@ createdb "regression$dbname1" || createdb_status=$?
createdb "regression$dbname2" || createdb_status=$?
createdb "regression$dbname3" || createdb_status=$?
-if "$MAKE" -C "$oldsrc" installcheck-parallel; then
+if "$MAKE" -C "$oldsrc" installcheck-parallel-notspsetup; then
oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
# before dumping, get rid of objects not existing in later versions
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 1a3164065f..b8681dbd17 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -137,7 +137,9 @@ check-tests: all tablespace-setup | temp-install
installcheck: all tablespace-setup
$(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_schedule $(EXTRA_TESTS)
-installcheck-parallel: all tablespace-setup
+installcheck-parallel: tablespace-setup installcheck-parallel-notspsetup
+
+installcheck-parallel-notspsetup: all
$(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
installcheck-tests: all tablespace-setup
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 74d37a31de..4516db1f38 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -115,7 +115,6 @@ sub installcheck_internal
"--no-locale");
push(@args, $maxconn) if $maxconn;
push(@args, @EXTRA_REGRESS_OPTS);
- CleanupTablespaceDirectory();
system(@args);
my $status = $? >> 8;
exit $status if $status;
@@ -125,6 +124,7 @@ sub installcheck_internal
sub installcheck
{
my $schedule = shift || 'serial';
+ CleanupTablespaceDirectory();
installcheck_internal($schedule);
return;
}
--
2.18.2
On Mon, May 11, 2020 at 05:13:54PM +0900, Kyotaro Horiguchi wrote:
> Tablespace directory cleanup is not done for all testing
> targets. Actually it is not done for the tools under bin/ except
> pg_upgrade.
Let's first take one problem at a time, as I can see that your patch
0002 is modifying a portion of what you added in 0001, and so let's
try to remove this WIN32 stuff from pg_regress.c.
+sub CleanupTablespaceDirectory
+{
+ my $tablespace = 'testtablespace';
+
+ rmtree($tablespace) if (-e $tablespace);
+ mkdir($tablespace);
+}
This check should use "-d" and not "-e" as it would be true for a file
as well. Also, in pg_regress.c, we remove the existing tablespace
test directory in --outputdir, which is "." by default but it can be a
custom one. Shouldn't you do the same logic in this new routine? So
we should have an optional argument for the output directory that
defaults to `pwd` if not defined, no? This means passing down the
argument only for upgradecheck() in vcregress.pl.
sub isolationcheck
{
chdir "../isolation";
+ CleanupTablespaceDirectory();
copy("../../../$Config/isolationtester/isolationtester.exe",
"../../../$Config/pg_isolation_regress");
my @args = (
[...]
print "============================================================\n";
print "Checking $module\n";
+ CleanupTablespaceDirectory();
my @args = (
"$topdir/$Config/pg_regress/pg_regress",
"--bindir=${topdir}/${Config}/psql",
I would put that just before the system() calls for consistency with
the rest.
--
Michael
Вложения
On Fri, May 15, 2020 at 11:58:55AM +0900, Michael Paquier wrote: > Let's first take one problem at a time, as I can see that your patch > 0002 is modifying a portion of what you added in 0001, and so let's > try to remove this WIN32 stuff from pg_regress.c. (Please note that this is not v13 material.) -- Michael
Вложения
Thank you for looking this!
At Fri, 15 May 2020 11:58:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Mon, May 11, 2020 at 05:13:54PM +0900, Kyotaro Horiguchi wrote:
> > Tablespace directory cleanup is not done for all testing
> > targets. Actually it is not done for the tools under bin/ except
> > pg_upgrade.
>
> Let's first take one problem at a time, as I can see that your patch
> 0002 is modifying a portion of what you added in 0001, and so let's
> try to remove this WIN32 stuff from pg_regress.c.
Yes, 0001 and 0001+0002 are alternatives. They should be merged if we
are going to fix the pg_upgrade test. I take this as we go on 0001+0002.
> +sub CleanupTablespaceDirectory
> +{
> + my $tablespace = 'testtablespace';
> +
> + rmtree($tablespace) if (-e $tablespace);
> + mkdir($tablespace);
> +}
> This check should use "-d" and not "-e" as it would be true for a file
> as well. Also, in pg_regress.c, we remove the existing tablespace
That was intentional so that a file with the name don't stop
testing. Actually pg_regress is checking only for a directory in other
place and it's not that bad since no-one can create a file with that
name while running test. On the other hand, is there any reason for
refraining from removing if it weren't a directory but a file?
Changed to -d in the attached.
> as well. Also, in pg_regress.c, we remove the existing tablespace
> test directory in --outputdir, which is "." by default but it can be a
> custom one. Shouldn't you do the same logic in this new routine? So
> we should have an optional argument for the output directory that
> defaults to `pwd` if not defined, no? This means passing down the
> argument only for upgradecheck() in vcregress.pl.
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.)
It is easy in perl scripts, but rather complex for makefiles. The
attached is using a perl one-liner to extract outputdir from
EXTRA_REGRESS_OPTS. I don't like that but I didn't come up with better
alternatives. On the other hand ActivePerl (with default
installation) doesn't seem to know Getopt::Long::GetOptions and
friends. In the attached vcregress.pl parses --outputdir not using
GetOpt::Long...
> sub isolationcheck
> {
> chdir "../isolation";
> + CleanupTablespaceDirectory();
> copy("../../../$Config/isolationtester/isolationtester.exe",
> "../../../$Config/pg_isolation_regress");
> my @args = (
> [...]
> print "============================================================\n";
> print "Checking $module\n";
> + CleanupTablespaceDirectory();
> my @args = (
> "$topdir/$Config/pg_regress/pg_regress",
> "--bindir=${topdir}/${Config}/psql",
> I would put that just before the system() calls for consistency with
> the rest.
Right. That's just an mistake. Fixed along with subdircheck.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From db38bd5cdbea950cdeba8bd4745801e9c0a2824c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 30 Apr 2020 14:06:51 +0900
Subject: [PATCH] Move tablespace cleanup out of pg_regress.
Tablespace directory is cleaned-up in regress/GNUmakefile for all
platforms other than Windows. For Windoiws pg_regress does that on
behalf of make. That is not only ugly also leads to do the clean up
twice at once. Let's move the cleanup code out of pg_regress into
vcregress.pl, where is more sutable place.
This also fixes a bug that the test for pg_upgrade mistakenly cleans
up the tablespace directory of default tmp-install location, instead
of its own installation directoty.
---
src/test/regress/GNUmakefile | 6 ++++--
src/test/regress/pg_regress.c | 22 ----------------------
src/tools/msvc/vcregress.pl | 25 +++++++++++++++++++++++++
3 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 1a3164065f..815d87904b 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -118,8 +118,10 @@ submake-contrib-spi: | submake-libpgport submake-generated-headers
.PHONY: tablespace-setup
tablespace-setup:
- rm -rf ./testtablespace
- mkdir ./testtablespace
+# extract --outputdir optsion from EXTRA_REGRESS_OPTS
+ $(eval dir := $(shell perl -e 'use Getopt::Long qw(GetOptionsFromString Configure); Configure("pass_through",
"gnu_getopt");($$r, $$x) = GetOptionsFromString($$ENV{"EXTRA_REGRESS_OPTS"}, "outputdir=s" => \$$dir); print ((defined
$$dir? $$dir : ".") . "/testtablespace");'))
+ rm -rf $(dir)
+ mkdir $(dir)
##
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 38b2b1e8e1..c56bfaf7f5 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -491,28 +491,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 0a98f6e37d..4bada14092 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -114,6 +114,13 @@ sub installcheck_internal
"--no-locale");
push(@args, $maxconn) if $maxconn;
push(@args, @EXTRA_REGRESS_OPTS);
+
+ my $outputdir;
+ foreach (@EXTRA_REGRESS_OPTS)
+ {
+ $outputdir = $1 if (/^ *--outputdir *= *(.+) *$/);
+ }
+ CleanupTablespaceDirectory($outputdir);
system(@args);
my $status = $? >> 8;
exit $status if $status;
@@ -143,6 +150,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;
@@ -169,6 +177,7 @@ sub ecpgcheck
"--no-locale",
"--temp-instance=./tmp_chk");
push(@args, $maxconn) if $maxconn;
+ CleanupTablespaceDirectory();
system(@args);
$status = $? >> 8;
exit $status if $status;
@@ -186,6 +195,7 @@ sub isolationcheck
"--inputdir=.",
"--schedule=./isolation_schedule");
push(@args, $maxconn) if $maxconn;
+ CleanupTablespaceDirectory();
system(@args);
my $status = $? >> 8;
exit $status if $status;
@@ -382,6 +392,7 @@ sub plcheck
"$topdir/$Config/pg_regress/pg_regress",
"--bindir=$topdir/$Config/psql",
"--dbname=pl_regression", @lang_args, @tests);
+ CleanupTablespaceDirectory();
system(@args);
my $status = $? >> 8;
exit $status if $status;
@@ -444,6 +455,7 @@ sub subdircheck
"--bindir=${topdir}/${Config}/psql",
"--dbname=contrib_regression", @opts, @tests);
print join(' ', @args), "\n";
+ CleanupTablespaceDirectory();
system(@args);
chdir "..";
return;
@@ -739,6 +751,19 @@ sub InstallTemp
return;
}
+sub CleanupTablespaceDirectory
+{
+ my ($testdir) = @_;
+
+ $testdir = cwd() if (! defined $testdir);
+
+ # don't bother checking trailing directory separator in $testdir
+ my $tablespace = "$testdir/testtablespace";
+
+ rmtree($tablespace) if (-d $tablespace);
+ mkdir($tablespace);
+}
+
sub usage
{
print STDERR
--
2.18.2
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.ntt@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
Вложения
Thanks for working on this. At Wed, 17 Jun 2020 16:12:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in > 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. It look good to me as the Windows part. I agree that vcregress.pl don't need to parse EXTRA_REGRESS_OPTS by allowing a bit more tight bond between the caller sites of pg_regress and pg_regress. > 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.ntt@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, Agreed on probability. > 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. Chaning to TAP sounds nice as a goal. As the next step we need to amend GNUmakefile not to cleanup tablespace for the four test items. Then somehow treat tablespaces at non-default place? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, Jun 17, 2020 at 05:02:31PM +0900, Kyotaro Horiguchi wrote: > It look good to me as the Windows part. I agree that vcregress.pl > don't need to parse EXTRA_REGRESS_OPTS by allowing a bit more tight > bond between the caller sites of pg_regress and pg_regress. Thanks, applied this part to HEAD then after more testing. > Chaining to TAP sounds nice as a goal. I submitted a patch for that, but we had no clear agreements about how to handle major upgrades, as this involves a somewhat large refactoring of PostgresNode.pm so as you register a path to the binaries used by a given node registered. > As the next step we need to amend GNUmakefile not to cleanup > tablespace for the four test items. Then somehow treat tablespaces at > non-default place? Ah, you mean to not reset testtablespace where that's not necessary in the tests by reworking the rules? Yeah, perhaps we could do something like that. Not sure yet how to shape that in term of code but if you have a clear idea, please feel free to submit it. I think that this may be better if discussed on a different thread though. -- Michael
Вложения
On Thu, Jun 18, 2020 at 1:42 PM Michael Paquier <michael@paquier.xyz> wrote: > Thanks, applied this part to HEAD then after more testing. Hmm, somehow this (well I guess it's this commit based on timing and the area it touches, not sure exactly why) made cfbot's Windows build fail, like this: --- C:/projects/postgresql/src/test/regress/expected/tablespace.out 2020-06-19 21:26:24.661817000 +0000 +++ C:/projects/postgresql/src/test/regress/results/tablespace.out 2020-06-19 21:26:28.613257500 +0000 @@ -2,83 +2,78 @@ CREATE TABLESPACE regress_tblspacewith LOCATION 'C:/projects/postgresql/src/test/regress/testtablespace' WITH (some_nonexistent_parameter = true); -- fail ERROR: unrecognized parameter "some_nonexistent_parameter" CREATE TABLESPACE regress_tblspacewith LOCATION 'C:/projects/postgresql/src/test/regress/testtablespace' WITH (random_page_cost = 3.0); -- ok +ERROR: could not set permissions on directory "C:/projects/postgresql/src/test/regress/testtablespace": Permission denied Any ideas? Here's what it does: https://github.com/macdice/cfbot/tree/master/appveyor
On Sat, Jun 20, 2020 at 09:33:26AM +1200, Thomas Munro wrote: > Hmm, somehow this (well I guess it's this commit based on timing and > the area it touches, not sure exactly why) made cfbot's Windows build > fail, like this: > > --- C:/projects/postgresql/src/test/regress/expected/tablespace.out > 2020-06-19 21:26:24.661817000 +0000 > +++ C:/projects/postgresql/src/test/regress/results/tablespace.out > 2020-06-19 21:26:28.613257500 +0000 > @@ -2,83 +2,78 @@ > CREATE TABLESPACE regress_tblspacewith LOCATION > 'C:/projects/postgresql/src/test/regress/testtablespace' WITH > (some_nonexistent_parameter = true); -- fail > ERROR: unrecognized parameter "some_nonexistent_parameter" > CREATE TABLESPACE regress_tblspacewith LOCATION > 'C:/projects/postgresql/src/test/regress/testtablespace' WITH > (random_page_cost = 3.0); -- ok > +ERROR: could not set permissions on directory > "C:/projects/postgresql/src/test/regress/testtablespace": Permission > denied > > Any ideas? Here's what it does: > > https://github.com/macdice/cfbot/tree/master/appveyor I am not sure, and I am not really familiar with this stuff. Your code does a simple vcregress check, and that should take care of automatically cleaning up the testtablespace path. The buildfarm uses this code for MSVC builds and does not complain, nor do my own VMs complain. A difference in the processing after 2b2a070d is that the tablespace cleanup/creation does not happen while holding a restricted token [1] anymore because it got out of pg_regress.c. Are there any kind of restrictions applied to the user running appveyor on Windows? [1]: https://docs.microsoft.com/en-us/windows/win32/secauthz/restricted-tokens -- Michael
Вложения
On Sat, Jun 20, 2020 at 2:42 PM Michael Paquier <michael@paquier.xyz> wrote: > > +ERROR: could not set permissions on directory > > "C:/projects/postgresql/src/test/regress/testtablespace": Permission > > denied > > > > Any ideas? Here's what it does: > > > > https://github.com/macdice/cfbot/tree/master/appveyor > > I am not sure, and I am not really familiar with this stuff. Your > code does a simple vcregress check, and that should take care of > automatically cleaning up the testtablespace path. The buildfarm uses > this code for MSVC builds and does not complain, nor do my own VMs > complain. A difference in the processing after 2b2a070d is that the > tablespace cleanup/creation does not happen while holding a restricted > token [1] anymore because it got out of pg_regress.c. Are there any > kind of restrictions applied to the user running appveyor on Windows? Thanks for the clue. Appveyor runs your build script as a privileged user (unlike, I assume, the build farm animals), and that has caused a problem with this test in the past, though I don't know the details. I might go and teach it to skip that test until a fix can be found.
On Sat, Jun 20, 2020 at 03:01:36PM +1200, Thomas Munro wrote: > Thanks for the clue. Appveyor runs your build script as a privileged > user (unlike, I assume, the build farm animals), and that has caused a > problem with this test in the past, though I don't know the details. > I might go and teach it to skip that test until a fix can be found. Thanks, I was not aware of that. Is it a fix that involves your code or something else? How long do you think it would take to address that? Another strategy that we could do is also a revert of 2b2a070 for now to allow the cfbot to go through and then register this thread in the CF app to allow the bot to pick it up and test it, so as there is more room to get a fix. The next CF is in ten days, so it would be annoying to reduce the automatic test coverage the cfbot provides :/ -- Michael
Вложения
On Sat, Jun 20, 2020 at 6:46 PM Michael Paquier <michael@paquier.xyz> wrote: > On Sat, Jun 20, 2020 at 03:01:36PM +1200, Thomas Munro wrote: > > Thanks for the clue. Appveyor runs your build script as a privileged > > user (unlike, I assume, the build farm animals), and that has caused a > > problem with this test in the past, though I don't know the details. > > I might go and teach it to skip that test until a fix can be found. > > Thanks, I was not aware of that. Is it a fix that involves your code > or something else? How long do you think it would take to address > that? Another strategy that we could do is also a revert of 2b2a070 > for now to allow the cfbot to go through and then register this thread > in the CF app to allow the bot to pick it up and test it, so as there > is more room to get a fix. The next CF is in ten days, so it would be > annoying to reduce the automatic test coverage the cfbot provides :/ I'm not sure what needs to change, but in the meantime I told it to comment out the offending test from the schedule files: +before_test: + - 'perl -p -i.bak -e "s/^test: tablespace/#test: tablespace/" src/test/regress/serial_schedule' + - 'perl -p -i.bak -e "s/^test: tablespace/#test: tablespace/" src/test/regress/parallel_schedule' Now the results are slowly turning green again.
On Sun, Jun 21, 2020 at 12:08:37PM +1200, Thomas Munro wrote: > I'm not sure what needs to change, but in the meantime I told it to > comment out the offending test from the schedule files: > > +before_test: > + - 'perl -p -i.bak -e "s/^test: tablespace/#test: tablespace/" > src/test/regress/serial_schedule' > + - 'perl -p -i.bak -e "s/^test: tablespace/#test: tablespace/" > src/test/regress/parallel_schedule' > > Now the results are slowly turning green again. Thanks, and sorry for the trouble. What actually happened back in 2018? I can see c2ff3c68 in the git history of the cfbot code, but it does not give much details. -- Michael
Вложения
On Sun, Jun 21, 2020 at 8:42 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Sun, Jun 21, 2020 at 12:08:37PM +1200, Thomas Munro wrote:
> > I'm not sure what needs to change, but in the meantime I told it to
> > comment out the offending test from the schedule files:
> >
> > +before_test:
> > + - 'perl -p -i.bak -e "s/^test: tablespace/#test: tablespace/"
> > src/test/regress/serial_schedule'
> > + - 'perl -p -i.bak -e "s/^test: tablespace/#test: tablespace/"
> > src/test/regress/parallel_schedule'
> >
> > Now the results are slowly turning green again.
>
> Thanks, and sorry for the trouble. What actually happened back in
> 2018? I can see c2ff3c68 in the git history of the cfbot code, but it
> does not give much details.
commit ce5d3424d6411f7a7228fd4463242cb382af3e0c
Author: Andrew Dunstan <andrew@dunslane.net>
Date: Sat Oct 20 09:02:36 2018 -0400
Lower privilege level of programs calling regression_main
On Windows this mean that the regression tests can now safely and
successfully run as Administrator, which is useful in situations like
Appveyor. Elsewhere it's a no-op.
Backpatch to 9.5 - this is harder in earlier branches and not worth the
trouble.
Discussion:
https://postgr.es/m/650b0c29-9578-8571-b1d2-550d7f89f307@2ndQuadrant.com
On Sun, Jun 21, 2020 at 10:38:22PM +1200, Thomas Munro wrote: > On Sun, Jun 21, 2020 at 8:42 PM Michael Paquier <michael@paquier.xyz> wrote: >> Thanks, and sorry for the trouble. What actually happened back in >> 2018? I can see c2ff3c68 in the git history of the cfbot code, but it >> does not give much details. > > commit ce5d3424d6411f7a7228fd4463242cb382af3e0c > Author: Andrew Dunstan <andrew@dunslane.net> > Date: Sat Oct 20 09:02:36 2018 -0400 > > Lower privilege level of programs calling regression_main > > On Windows this mean that the regression tests can now safely and > successfully run as Administrator, which is useful in situations like > Appveyor. Elsewhere it's a no-op. > > Backpatch to 9.5 - this is harder in earlier branches and not worth the > trouble. > > Discussion: > https://postgr.es/m/650b0c29-9578-8571-b1d2-550d7f89f307@2ndQuadrant.com Thanks for the reference. This also means that as much as I'd like to keep the recreation of testtablespace out of pg_regress for consistency, 2b2a070 has also broken a case we have claimed to support since ce5d342. A bit of digging around I have found this case from a guy of Yandex, visibly running our regression test suite: https://help.appveyor.com/discussions/questions/1888-running-tests-with-reduced-privileges And the conclusion seems like it is not really possible to do that within appveyor, using a trick with openssh to manipulate privileges as wanted, as referenced here: https://github.com/yandex-qatools/postgresql-embedded At the end of the day, it looks more simple to me to just revert 2b2a070 if we just want to keep your stuff running without extra workload from your side. Extra opinions are welcome. -- Michael
Вложения
Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Jun 18, 2020 at 1:42 PM Michael Paquier <michael@paquier.xyz> wrote:
>> Thanks, applied this part to HEAD then after more testing.
> Hmm, somehow this (well I guess it's this commit based on timing and
> the area it touches, not sure exactly why) made cfbot's Windows build
> fail, like this:
Should now be possible to undo whatever hack you had to use ...
regards, tom lane
On Fri, Jul 10, 2020 at 09:35:56AM -0400, Tom Lane wrote: > Should now be possible to undo whatever hack you had to use ... Yes, I have also opened an issue on github: https://github.com/macdice/cfbot/issues/11/ -- Michael
Вложения
On Sat, Jul 11, 2020 at 10:35:07AM +0900, Michael Paquier wrote: > Yes, I have also opened an issue on github: > https://github.com/macdice/cfbot/issues/11/ And Thomas has just fixed it: https://github.com/macdice/cfbot/commit/e78438444a00bc8d83863645503b2f7c1a9da016 -- Michael