Thread: Re: [HACKERS] float8 regression failure (HEAD, cygwin)
On 20/07/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Reini Urban <rurban@x-ray.at> writes:
> > BTW: HAVE_LONG_LONG_INT_64 is defined, so INT64_IS_BUSTED is defined also.
>
> You sure? INT64_IS_BUSTED should *not* be set in that case --- it's
> only supposed to be set if we couldn't find any 64-bit-int type at all.
>
> As for the regression test failure, it's odd because it looks to me that
> the actual test output is an exact match to the default "float8.out"
> file. I'm not sure why pg_regress chose to report a diff against
> float8-small-is-zero.out instead. This may be another teething pain
> of the new pg_regress-in-C code --- could you trace through it and see
> what's happening?
Apparently the regression test is comparing the results/float8.out
with expected/float8-small-is-zero.out because of the following line
in
src/test/regress/resultmap :
float8/i.86-pc-cygwin=float8-small-is-zero
I've changed that line to :
float8/i.86-pc-cygwin=float8
and the regression test ended successfully : "All 100 tests passed."
I don't know why there are several expected results for the float8 test,
depending on the platform. Is the modification ok?
I've attached the patch, and cc'ed to pgsql-patches.
Cheers,
Adrian Maier
Adrian Maier wrote: > On 20/07/06, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Reini Urban <rurban@x-ray.at> writes: >> > BTW: HAVE_LONG_LONG_INT_64 is defined, so INT64_IS_BUSTED is >> defined also. >> >> You sure? INT64_IS_BUSTED should *not* be set in that case --- it's >> only supposed to be set if we couldn't find any 64-bit-int type at all. >> >> As for the regression test failure, it's odd because it looks to me that >> the actual test output is an exact match to the default "float8.out" >> file. I'm not sure why pg_regress chose to report a diff against >> float8-small-is-zero.out instead. This may be another teething pain >> of the new pg_regress-in-C code --- could you trace through it and see >> what's happening? > > Apparently the regression test is comparing the results/float8.out > with expected/float8-small-is-zero.out because of the following line > in > src/test/regress/resultmap : > float8/i.86-pc-cygwin=float8-small-is-zero > > I've changed that line to : > float8/i.86-pc-cygwin=float8 > and the regression test ended successfully : "All 100 tests passed." > > I don't know why there are several expected results for the float8 test, > depending on the platform. Is the modification ok? > > I've attached the patch, and cc'ed to pgsql-patches. > The problem with this is that we have another Cygwin member on buildfarm which passes the tests happily, and will thus presumably fail if we make this patch. You are running Cygwin 1.5.21 and the other buildfarm member is running 1.5.19, so that is possibly the difference. Maybe we need to abandon trying to map float8 results exactly in the resultmap file, and just let pg_regress pick the best fit as we do with some other tests. cheers andrew
On 01/08/06, Andrew Dunstan <andrew@dunslane.net> wrote: > Adrian Maier wrote: > > On 20/07/06, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Apparently the regression test is comparing the results/float8.out > > with expected/float8-small-is-zero.out because of the following line > > in > > src/test/regress/resultmap : > > float8/i.86-pc-cygwin=float8-small-is-zero > > > > I've changed that line to : > > float8/i.86-pc-cygwin=float8 > > and the regression test ended successfully : "All 100 tests passed." > > > > I don't know why there are several expected results for the float8 test, > > depending on the platform. Is the modification ok? > > > > I've attached the patch, and cc'ed to pgsql-patches. > > The problem with this is that we have another Cygwin member on buildfarm > which passes the tests happily, and will thus presumably fail if we make > this patch. You are running Cygwin 1.5.21 and the other buildfarm member > is running 1.5.19, so that is possibly the difference. This is indeed a problem. It would be difficult or even impossible to use different expected results for different versions of cygwin. > Maybe we need to abandon trying to map float8 results exactly in the > resultmap file, and just let pg_regress pick the best fit as we do with > some other tests. Oh, is it possible to do that? That sounds great. Which other tests work like that? Cheers, Adrian Maier
[ re cassowary buildfarm failure ]
"Adrian Maier" <adrian.maier@gmail.com> writes:
> On 20/07/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As for the regression test failure, it's odd because it looks to me that
>> the actual test output is an exact match to the default "float8.out"
>> file. I'm not sure why pg_regress chose to report a diff against
>> float8-small-is-zero.
> Apparently the regression test is comparing the results/float8.out
> with expected/float8-small-is-zero.out because of the following line
> in
> src/test/regress/resultmap :
> float8/i.86-pc-cygwin=float8-small-is-zero
Doh ... the question though is why are you getting different results
from everybody else? There are other cygwin machines in the buildfarm
and they are all passing regression --- I suppose they'd start failing
if we remove that resultmap entry.
The regular float8 result is certainly "more correct" than
float8-small-is-zero, so I'm all for removing the resultmap entry
if we can do it. But we'd need to be able to explain to people how
to get their machines to pass, and right now I don't know what to say.
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
> Maybe we need to abandon trying to map float8 results exactly in the
> resultmap file, and just let pg_regress pick the best fit as we do with
> some other tests.
I thought about that too but it seems a very bad idea. small-is-zero is
distinctly "less correct" than the regular output, and I don't think we
want pg_regress to be blindly accepting it as OK on any platform.
Perhaps we could stick a version check into the resultmap lookup? It'd
likely have been painful on the shell script implementation but now that
the code is in C I think we have lots of flexibility. There's no need
to feel bound by the historical resultmap format.
However this is all premature unless we can verify that "cgywin's strtod()
complains about float underflow after version so-and-so". Do they
publish a detailed change log?
regards, tom lane
On 01/08/06, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > Maybe we need to abandon trying to map float8 results exactly in the > > resultmap file, and just let pg_regress pick the best fit as we do with > > some other tests. > > I thought about that too but it seems a very bad idea. small-is-zero is > distinctly "less correct" than the regular output, and I don't think we > want pg_regress to be blindly accepting it as OK on any platform. > > Perhaps we could stick a version check into the resultmap lookup? It'd > likely have been painful on the shell script implementation but now that > the code is in C I think we have lots of flexibility. There's no need > to feel bound by the historical resultmap format. > > However this is all premature unless we can verify that "cgywin's strtod() > complains about float underflow after version so-and-so". Do they > publish a detailed change log? There are links to the last few releases on their home page http://www.cygwin.com , in the News section. -- Adrian Maier
Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> One other thought I had was that we could have
>> pg_regress always allow a fallback to the canonical result file.
>>
>
> Hm, that's a good thought. Want to see how painful it is to code?
>
>
Would this do the trick?
cheers
andrew
Index: pg_regress.c
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/pg_regress.c,v
retrieving revision 1.16
diff -c -r1.16 pg_regress.c
*** pg_regress.c 27 Jul 2006 15:37:19 -0000 1.16
--- pg_regress.c 1 Aug 2006 14:04:20 -0000
***************
*** 914,919 ****
--- 914,952 ----
}
}
+ /*
+ * fall back on the canonical results file if we haven't tried it yet
+ * and haven't found a complete match yet.
+ */
+
+ if (strcmp(expectname, testname) != 0)
+ {
+ snprintf(expectfile, sizeof(expectfile), "%s/expected/%s.out",
+ inputdir, testname, i);
+ if (!file_exists(expectfile))
+ continue;
+
+ snprintf(cmd, sizeof(cmd),
+ SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
+ basic_diff_opts, expectfile, resultsfile, diff);
+ run_diff(cmd);
+
+ if (file_size(diff) == 0)
+ {
+ /* No diff = no changes = good */
+ unlink(diff);
+ return false;
+ }
+
+ l = file_line_count(diff);
+ if (l < best_line_count)
+ {
+ /* This diff was a better match than the last one */
+ best_line_count = l;
+ strcpy(best_expect_file, expectfile);
+ }
+ }
+
/*
* Use the best comparison file to generate the "pretty" diff, which
* we append to the diffs summary file.
Andrew Dunstan <andrew@dunslane.net> writes:
> Would this do the trick?
I think Bruce changed the call convention for run_diff ... are you
looking at CVS tip? Otherwise it looks reasonable.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Would this do the trick? >> > > I think Bruce changed the call convention for run_diff ... are you > looking at CVS tip? Otherwise it looks reasonable. > > You're right. I had forgotten to do a cvs update. Fixed and committed. cheers andrew