Move regression.diffs of pg_upgrade test suite

On 12/23/18 10:44 PM, Noah Misch wrote:

src/bin/pg_upgrade/test.sh runs installcheck, which writes to files in
src/test/regress. This has at least two disadvantages when check-world runs
both this test suite and the "make check" suite:

1. The suite finishing second will overwrite the other's regression.{out,diffs}.
2. If these suites run a given test file in parallel (possible with "make -j
check-world"), they simultaneously edit a file in src/test/regress/results.
This can cause reporting of spurious failures. On my system, the symptom
is a regression.diffs indicating that the .out file contained ranges of NUL
bytes (holes) and/or lacked expected lines.

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.

I think the right fix, attached, is to use "pg_regress --outputdir" to
redirect these files to src/bin/pg_upgrade/tmp_check/regress. I chose that
particular path because it will still fit naturally if we ever rewrite test.sh
using src/test/perl. I'm recommending that the buildfarm capture[1] files
matching src/bin/pg_upgrade/tmp_check/*/*.diffs, which will work even if we
make this test suite run installcheck more than once. This revealed a few
places where tests assume @abs_builddir@ is getcwd(), which I fixed.

Thanks,
nm

[1] https://github.com/PGBuildFarm/client-code/blob/REL_9/PGBuild/Modules/TestUpgrade.pm#L126

Seems reasonable.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

I think the right fix, attached, is to use "pg_regress --outputdir" to
redirect these files to src/bin/pg_upgrade/tmp_check/regress.

Seems reasonable.

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.

regards, tom lane

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.

I think the right fix, attached, is to use "pg_regress --outputdir" to
redirect these files to src/bin/pg_upgrade/tmp_check/regress.

Seems reasonable.

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
index 19b48b3..dfff17f 100644
--- a/PGBuild/Modules/TestUpgrade.pm
+++ b/PGBuild/Modules/TestUpgrade.pm
@@ -117,11 +117,16 @@ sub check
         @checklog = run_log($cmd);
     }

+    # Pre-2019 runs could create src/test/regress/regression.diffs.  Its
+    # inclusion is a harmless no-op for later runs; if another stage
+    # (e.g. make_check()) failed and created that file, the run ends before
+    # reaching this stage.
     my @logfiles = glob(
         "$self->{pgsql}/contrib/pg_upgrade/*.log
          $self->{pgsql}/contrib/pg_upgrade/log/*
          $self->{pgsql}/src/bin/pg_upgrade/*.log
          $self->{pgsql}/src/bin/pg_upgrade/log/*
+         $self->{pgsql}/src/bin/pg_upgrade/tmp_check/*/*.diffs
          $self->{pgsql}/src/test/regress/*.diffs"
     );
     foreach my $log (@logfiles)

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.

I think the right fix, attached, is to use "pg_regress --outputdir" to
redirect these files to src/bin/pg_upgrade/tmp_check/regress.

Seems reasonable.

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
index 19b48b3..dfff17f 100644
--- a/PGBuild/Modules/TestUpgrade.pm
+++ b/PGBuild/Modules/TestUpgrade.pm
@@ -117,11 +117,16 @@ sub check
@checklog = run_log($cmd);
}

+    # Pre-2019 runs could create src/test/regress/regression.diffs.  Its
+    # inclusion is a harmless no-op for later runs; if another stage
+    # (e.g. make_check()) failed and created that file, the run ends before
+    # reaching this stage.
my @logfiles = glob(
"$self->{pgsql}/contrib/pg_upgrade/*.log
$self->{pgsql}/contrib/pg_upgrade/log/*
$self->{pgsql}/src/bin/pg_upgrade/*.log
$self->{pgsql}/src/bin/pg_upgrade/log/*
+         $self->{pgsql}/src/bin/pg_upgrade/tmp_check/*/*.diffs
$self->{pgsql}/src/test/regress/*.diffs"
);
foreach my $log (@logfiles)

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.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

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.