Thread: Move regression.diffs of pg_upgrade test suite
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
Attachment
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.