From: Noah Misch <noah@leadboat.com>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Move regression.diffs of pg_upgrade test suite
Date: Sun, 19 May 2019 18:24:36 -0700
Message-ID: <20190520012436.GA1480421@rfd.leadboat.com> (raw)
In-Reply-To: <20181230162856.GA187973@gust.leadboat.com>
References: <20181224034411.GA3224776@rfd.leadboat.com>
<ede9f7a3-2082-ce59-abb0-fe867dc03bbb@2ndQuadrant.com>
<22708.1545861757@sss.pgh.pa.us>
<20181226224428.GA3277716@rfd.leadboat.com>
<37ead3b7-b4a9-5a1f-6a10-c6ab9eb0d2aa@2ndQuadrant.com>
<20181230162856.GA187973@gust.leadboat.com>
On Sun, Dec 30, 2018 at 11:28:56AM -0500, Noah Misch wrote:
> On Sun, Dec 30, 2018 at 10:41:46AM -0500, Andrew Dunstan wrote:
> > On 12/26/18 5:44 PM, Noah Misch wrote:
> > > On Wed, Dec 26, 2018 at 05:02:37PM -0500, Tom Lane wrote:
> > >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> > >>> On 12/23/18 10:44 PM, Noah Misch wrote:
> > >>>> A disadvantage of any change here is that it degrades buildfarm reports, which
> > >>>> recover slowly as owners upgrade to a fixed buildfarm release. This will be
> > >>>> similar to the introduction of --outputdir=output_iso. On non-upgraded
> > >>>> animals, pg_upgradeCheck failures will omit regression.diffs.
>
> > >> Do we need to change anything in the buildfarm client to improve its
> > >> response to this? If so, seems like it might be advisable to make a
> > >> buildfarm release with the upgrade before committing the change.
> > >> Sure, not all owners will update right away, but if they don't even
> > >> have the option then we're not in a good place.
> > >
> > > It would have been convenient if, for each test target, PostgreSQL code
> > > decides the list of interesting log files and presents that list for the
> > > buildfarm client to consume. It's probably overkill to redesign that now,
> > > though. I also don't think it's of top importance to have unbroken access to
> > > this regression.diffs, because defects that cause this run to fail will
> > > eventually upset "install-check-C" and/or "check". Even so, it's fine to
> > > patch the buildfarm client in advance of the postgresql.git change:
> > >
> > > diff --git a/PGBuild/Modules/TestUpgrade.pm b/PGBuild/Modules/TestUpgrade.pm
>
> > I'll commit this or something similar, but I generally try not to make
> > new releases more frequently than once every 3 months, and it's only six
> > weeks since the last release. So unless there's a very good reason I am
> > not planning on a release before February.
>
> There's no rush; I don't recall other reports of the spurious failure
> described in the original post. I'll plan to push the postgresql.git change
> around 2019-03-31, so animals updating within a month of release will have no
> degraded pg_upgradeCheck failure reports.
The buildfarm release landed 2019-04-04, so I pushed $SUBJECT today, in commit
bd1592e. The buildfarm was unanimous against it, for two reasons. First, the
patch was incompatible with NO_TEMP_INSTALL=1, which the buildfarm uses. In a
normal "make -C src/bin/pg_upgrade check", the act of creating the temporary
installation also creates "tmp_check". With NO_TEMP_INSTALL=1, it's instead
the initdb that creates "tmp_check". I plan to fix that by removing and
creating "tmp_check" early. This fixes another longstanding bug; a rerun of
"vcregress upgradecheck" would fail with 'directory "[...]/tmp_check/data"
exists but is not empty'. It's also more consistent with $(prove_check),
eliminates the possibility that a file in "tmp_check" survives from an earlier
run, and ends NO_TEMP_INSTALL=1 changing the "tmp_check" creation umask.
Second, I broke "vcregress installcheck" by writing "funcname $arg" where
funcname was declared later in the file. Neither the function invocation
style nor the function declaration order were in line with that file's style,
so I'm changing both.
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.shindex 66e69b9..cca9b4f 100644--- a/src/bin/pg_upgrade/test.sh+++ b/src/bin/pg_upgrade/test.sh@@ -68,6 +68,8 @@ export PGHOST
# don't rely on $PWD here, as old shells don't set it
temp_root=`pwd`/tmp_check
+rm -rf "$temp_root"+mkdir "$temp_root"
if [ "$1" = '--install' ]; then
temp_install=$temp_root/install
@@ -108,7 +110,6 @@ export PATH
BASE_PGDATA="$temp_root/data"
PGDATA="${BASE_PGDATA}.old"
export PGDATA
-rm -rf "$BASE_PGDATA" "$PGDATA"
logdir=`pwd`/log
rm -rf "$logdir"
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.plindex 2aa29ab..25814c6 100644--- a/src/tools/msvc/vcregress.pl+++ b/src/tools/msvc/vcregress.pl@@ -546,7 +546,8 @@ sub upgradecheck
$ENV{PGHOST} = 'localhost';
$ENV{PGPORT} ||= 50432;
my $tmp_root = "$topdir/src/bin/pg_upgrade/tmp_check";
- (mkdir $tmp_root || die $!) unless -d $tmp_root;+ rmtree($tmp_root);+ mkdir $tmp_root || die $!;
my $upg_tmp_install = "$tmp_root/install"; # unshared temp install
print "Setting up temp install\n\n";
Install($upg_tmp_install, "all", $config);
@@ -559,7 +560,8 @@ sub upgradecheck
my $data = "$tmp_root/data";
$ENV{PGDATA} = "$data.old";
my $logdir = "$topdir/src/bin/pg_upgrade/log";
- (mkdir $logdir || die $!) unless -d $logdir;+ rmtree($logdir);+ mkdir $logdir || die $!;
print "\nRunning initdb on old cluster\n\n";
standard_initdb() or exit 1;
print "\nStarting old cluster\n\n";
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.shindex cca9b4f..e5e91c3 100644--- a/src/bin/pg_upgrade/test.sh+++ b/src/bin/pg_upgrade/test.sh@@ -111,6 +111,17 @@ BASE_PGDATA="$temp_root/data"
PGDATA="${BASE_PGDATA}.old"
export PGDATA
+# 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"+export EXTRA_REGRESS_OPTS+mkdir "$outputdir"+mkdir "$outputdir"/sql+mkdir "$outputdir"/expected+mkdir "$outputdir"/testtablespace+
logdir=`pwd`/log
rm -rf "$logdir"
mkdir "$logdir"
diff --git a/src/test/regress/input/largeobject.source b/src/test/regress/input/largeobject.sourceindex b7a9d05..7e45b11 100644--- a/src/test/regress/input/largeobject.source+++ b/src/test/regress/input/largeobject.source@@ -203,12 +203,12 @@ END;
SELECT lo_export(loid, '@abs_builddir@/results/lotest.txt') FROM lotest_stash_values;
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid :LASTOID
-- just make sure \lo_export does not barf
-\lo_export :newloid 'results/lotest2.txt'+\lo_export :newloid '@abs_builddir@/results/lotest2.txt'
-- This is a hack to test that export/import are reversible
-- This uses knowledge about the inner workings of large object mechanism
@@ -223,7 +223,7 @@ TRUNCATE lotest_stash_values;
\lo_unlink :newloid
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid_1 :LASTOID
diff --git a/src/test/regress/output/largeobject.source b/src/test/regress/output/largeobject.sourceindex e29f542..761d7ff 100644--- a/src/test/regress/output/largeobject.source+++ b/src/test/regress/output/largeobject.source@@ -385,10 +385,10 @@ SELECT lo_export(loid, '@abs_builddir@/results/lotest.txt') FROM lotest_stash_va
1
(1 row)
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid :LASTOID
-- just make sure \lo_export does not barf
-\lo_export :newloid 'results/lotest2.txt'+\lo_export :newloid '@abs_builddir@/results/lotest2.txt'
-- This is a hack to test that export/import are reversible
-- This uses knowledge about the inner workings of large object mechanism
-- which should not be used outside it. This makes it a HACK
@@ -407,7 +407,7 @@ SELECT lo_unlink(loid) FROM lotest_stash_values;
TRUNCATE lotest_stash_values;
\lo_unlink :newloid
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid_1 :LASTOID
SELECT lo_from_bytea(0, lo_get(:newloid_1)) AS newloid_2
\gset
diff --git a/src/test/regress/output/largeobject_1.source b/src/test/regress/output/largeobject_1.sourceindex 6fd8cbe..7de3e7e 100644--- a/src/test/regress/output/largeobject_1.source+++ b/src/test/regress/output/largeobject_1.source@@ -385,10 +385,10 @@ SELECT lo_export(loid, '@abs_builddir@/results/lotest.txt') FROM lotest_stash_va
1
(1 row)
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid :LASTOID
-- just make sure \lo_export does not barf
-\lo_export :newloid 'results/lotest2.txt'+\lo_export :newloid '@abs_builddir@/results/lotest2.txt'
-- This is a hack to test that export/import are reversible
-- This uses knowledge about the inner workings of large object mechanism
-- which should not be used outside it. This makes it a HACK
@@ -407,7 +407,7 @@ SELECT lo_unlink(loid) FROM lotest_stash_values;
TRUNCATE lotest_stash_values;
\lo_unlink :newloid
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid_1 :LASTOID
SELECT lo_from_bytea(0, lo_get(:newloid_1)) AS newloid_2
\gset
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.plindex 25814c6..a2c8137 100644--- a/src/tools/msvc/vcregress.pl+++ b/src/tools/msvc/vcregress.pl@@ -99,9 +99,9 @@ exit 0;
########################################################################
-sub installcheck+sub installcheck_internal
{
- my $schedule = shift || 'serial';+ my ($schedule, @EXTRA_REGRESS_OPTS) = @_;
my @args = (
"../../../$Config/pg_regress/pg_regress",
"--dlpath=.",
@@ -111,12 +111,20 @@ sub installcheck
"--encoding=SQL_ASCII",
"--no-locale");
push(@args, $maxconn) if $maxconn;
+ push(@args, @EXTRA_REGRESS_OPTS);
system(@args);
my $status = $? >> 8;
exit $status if $status;
return;
}
+sub installcheck+{+ my $schedule = shift || 'serial';+ installcheck_internal($schedule);+ return;+}+
sub check
{
my $schedule = shift || 'parallel';
@@ -559,6 +567,13 @@ sub upgradecheck
$ENV{PATH} = "$bindir;$ENV{PATH}";
my $data = "$tmp_root/data";
$ENV{PGDATA} = "$data.old";
+ my $outputdir = "$tmp_root/regress";+ my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");+ mkdir "$outputdir" || die $!;+ mkdir "$outputdir/sql" || die $!;+ mkdir "$outputdir/expected" || die $!;+ mkdir "$outputdir/testtablespace" || die $!;+
my $logdir = "$topdir/src/bin/pg_upgrade/log";
rmtree($logdir);
mkdir $logdir || die $!;
@@ -574,7 +589,7 @@ sub upgradecheck
generate_db('', 91, 127, '');
print "\nSetting up data for upgrading\n\n";
- installcheck('parallel');+ installcheck_internal('parallel', @EXTRA_REGRESS_OPTS);
# now we can chdir into the source dir
chdir "$topdir/src/bin/pg_upgrade";
Attachments:
[text/plain] rm-upgrade-temp-root-v1.patch (1.5K, ../20190520012436.GA1480421@rfd.leadboat.com/2-rm-upgrade-temp-root-v1.patch)
download | inline diff:diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.shindex 66e69b9..cca9b4f 100644--- a/src/bin/pg_upgrade/test.sh+++ b/src/bin/pg_upgrade/test.sh@@ -68,6 +68,8 @@ export PGHOST
# don't rely on $PWD here, as old shells don't set it
temp_root=`pwd`/tmp_check
+rm -rf "$temp_root"+mkdir "$temp_root"
if [ "$1" = '--install' ]; then
temp_install=$temp_root/install
@@ -108,7 +110,6 @@ export PATH
BASE_PGDATA="$temp_root/data"
PGDATA="${BASE_PGDATA}.old"
export PGDATA
-rm -rf "$BASE_PGDATA" "$PGDATA"
logdir=`pwd`/log
rm -rf "$logdir"
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.plindex 2aa29ab..25814c6 100644--- a/src/tools/msvc/vcregress.pl+++ b/src/tools/msvc/vcregress.pl@@ -546,7 +546,8 @@ sub upgradecheck
$ENV{PGHOST} = 'localhost';
$ENV{PGPORT} ||= 50432;
my $tmp_root = "$topdir/src/bin/pg_upgrade/tmp_check";
- (mkdir $tmp_root || die $!) unless -d $tmp_root;+ rmtree($tmp_root);+ mkdir $tmp_root || die $!;
my $upg_tmp_install = "$tmp_root/install"; # unshared temp install
print "Setting up temp install\n\n";
Install($upg_tmp_install, "all", $config);
@@ -559,7 +560,8 @@ sub upgradecheck
my $data = "$tmp_root/data";
$ENV{PGDATA} = "$data.old";
my $logdir = "$topdir/src/bin/pg_upgrade/log";
- (mkdir $logdir || die $!) unless -d $logdir;+ rmtree($logdir);+ mkdir $logdir || die $!;
print "\nRunning initdb on old cluster\n\n";
standard_initdb() or exit 1;
print "\nStarting old cluster\n\n";
[text/plain] outputdir-pg_upgrade-v2.patch (5.5K, ../20190520012436.GA1480421@rfd.leadboat.com/3-outputdir-pg_upgrade-v2.patch)
download | inline diff:diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.shindex cca9b4f..e5e91c3 100644--- a/src/bin/pg_upgrade/test.sh+++ b/src/bin/pg_upgrade/test.sh@@ -111,6 +111,17 @@ BASE_PGDATA="$temp_root/data"
PGDATA="${BASE_PGDATA}.old"
export PGDATA
+# 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"+export EXTRA_REGRESS_OPTS+mkdir "$outputdir"+mkdir "$outputdir"/sql+mkdir "$outputdir"/expected+mkdir "$outputdir"/testtablespace+
logdir=`pwd`/log
rm -rf "$logdir"
mkdir "$logdir"
diff --git a/src/test/regress/input/largeobject.source b/src/test/regress/input/largeobject.sourceindex b7a9d05..7e45b11 100644--- a/src/test/regress/input/largeobject.source+++ b/src/test/regress/input/largeobject.source@@ -203,12 +203,12 @@ END;
SELECT lo_export(loid, '@abs_builddir@/results/lotest.txt') FROM lotest_stash_values;
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid :LASTOID
-- just make sure \lo_export does not barf
-\lo_export :newloid 'results/lotest2.txt'+\lo_export :newloid '@abs_builddir@/results/lotest2.txt'
-- This is a hack to test that export/import are reversible
-- This uses knowledge about the inner workings of large object mechanism
@@ -223,7 +223,7 @@ TRUNCATE lotest_stash_values;
\lo_unlink :newloid
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid_1 :LASTOID
diff --git a/src/test/regress/output/largeobject.source b/src/test/regress/output/largeobject.sourceindex e29f542..761d7ff 100644--- a/src/test/regress/output/largeobject.source+++ b/src/test/regress/output/largeobject.source@@ -385,10 +385,10 @@ SELECT lo_export(loid, '@abs_builddir@/results/lotest.txt') FROM lotest_stash_va
1
(1 row)
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid :LASTOID
-- just make sure \lo_export does not barf
-\lo_export :newloid 'results/lotest2.txt'+\lo_export :newloid '@abs_builddir@/results/lotest2.txt'
-- This is a hack to test that export/import are reversible
-- This uses knowledge about the inner workings of large object mechanism
-- which should not be used outside it. This makes it a HACK
@@ -407,7 +407,7 @@ SELECT lo_unlink(loid) FROM lotest_stash_values;
TRUNCATE lotest_stash_values;
\lo_unlink :newloid
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid_1 :LASTOID
SELECT lo_from_bytea(0, lo_get(:newloid_1)) AS newloid_2
\gset
diff --git a/src/test/regress/output/largeobject_1.source b/src/test/regress/output/largeobject_1.sourceindex 6fd8cbe..7de3e7e 100644--- a/src/test/regress/output/largeobject_1.source+++ b/src/test/regress/output/largeobject_1.source@@ -385,10 +385,10 @@ SELECT lo_export(loid, '@abs_builddir@/results/lotest.txt') FROM lotest_stash_va
1
(1 row)
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid :LASTOID
-- just make sure \lo_export does not barf
-\lo_export :newloid 'results/lotest2.txt'+\lo_export :newloid '@abs_builddir@/results/lotest2.txt'
-- This is a hack to test that export/import are reversible
-- This uses knowledge about the inner workings of large object mechanism
-- which should not be used outside it. This makes it a HACK
@@ -407,7 +407,7 @@ SELECT lo_unlink(loid) FROM lotest_stash_values;
TRUNCATE lotest_stash_values;
\lo_unlink :newloid
-\lo_import 'results/lotest.txt'+\lo_import '@abs_builddir@/results/lotest.txt'
\set newloid_1 :LASTOID
SELECT lo_from_bytea(0, lo_get(:newloid_1)) AS newloid_2
\gset
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.plindex 25814c6..a2c8137 100644--- a/src/tools/msvc/vcregress.pl+++ b/src/tools/msvc/vcregress.pl@@ -99,9 +99,9 @@ exit 0;
########################################################################
-sub installcheck+sub installcheck_internal
{
- my $schedule = shift || 'serial';+ my ($schedule, @EXTRA_REGRESS_OPTS) = @_;
my @args = (
"../../../$Config/pg_regress/pg_regress",
"--dlpath=.",
@@ -111,12 +111,20 @@ sub installcheck
"--encoding=SQL_ASCII",
"--no-locale");
push(@args, $maxconn) if $maxconn;
+ push(@args, @EXTRA_REGRESS_OPTS);
system(@args);
my $status = $? >> 8;
exit $status if $status;
return;
}
+sub installcheck+{+ my $schedule = shift || 'serial';+ installcheck_internal($schedule);+ return;+}+
sub check
{
my $schedule = shift || 'parallel';
@@ -559,6 +567,13 @@ sub upgradecheck
$ENV{PATH} = "$bindir;$ENV{PATH}";
my $data = "$tmp_root/data";
$ENV{PGDATA} = "$data.old";
+ my $outputdir = "$tmp_root/regress";+ my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");+ mkdir "$outputdir" || die $!;+ mkdir "$outputdir/sql" || die $!;+ mkdir "$outputdir/expected" || die $!;+ mkdir "$outputdir/testtablespace" || die $!;+
my $logdir = "$topdir/src/bin/pg_upgrade/log";
rmtree($logdir);
mkdir $logdir || die $!;
@@ -574,7 +589,7 @@ sub upgradecheck
generate_db('', 91, 127, '');
print "\nSetting up data for upgrading\n\n";
- installcheck('parallel');+ installcheck_internal('parallel', @EXTRA_REGRESS_OPTS);
# now we can chdir into the source dir
chdir "$topdir/src/bin/pg_upgrade";
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, andrew.dunstan@2ndquadrant.com, tgl@sss.pgh.pa.us
Subject: Re: Move regression.diffs of pg_upgrade test suite
In-Reply-To: <20190520012436.GA1480421@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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