Thread: float8 regression failure (HEAD, cygwin)
Hello,
While setting up a buildfarm installation for cygwin, I've
uncountered the following
regression failure :
float8 ... FAILED
================== pgsql.3132/src/test/regress/regression.diffs
*** ./expected/float8-small-is-zero.out Tue Jul 18 09:24:52 2006
--- ./results/float8.out Tue Jul 18 09:53:42 2006
***************
*** 13,29 **** SELECT '-10e400'::float8; ERROR: "-10e400" is out of range for type double precision SELECT
'10e-400'::float8;
! float8
! --------
! 0
! (1 row)
! SELECT '-10e-400'::float8;
! float8
! --------
! -0
! (1 row)
! -- bad input INSERT INTO FLOAT8_TBL(f1) VALUES (''); ERROR: invalid input syntax for type double precision: ""
--- 13,21 ---- SELECT '-10e400'::float8; ERROR: "-10e400" is out of range for type double precision SELECT
'10e-400'::float8;
! ERROR: "10e-400" is out of range for type double precision SELECT '-10e-400'::float8;
! ERROR: "-10e-400" is out of range for type double precision -- bad input INSERT INTO FLOAT8_TBL(f1) VALUES ('');
ERROR: invalid input syntax for type double precision: ""
***************
*** 377,383 ****
--- 369,377 ---- INSERT INTO FLOAT8_TBL(f1) VALUES ('-10e400'); ERROR: "-10e400" is out of range for type double
precisionINSERT INTO FLOAT8_TBL(f1) VALUES ('10e-400');
+ ERROR: "10e-400" is out of range for type double precision INSERT INTO FLOAT8_TBL(f1) VALUES ('-10e-400');
+ ERROR: "-10e-400" is out of range for type double precision -- maintain external table consistency across platforms
--delete all values and reinsert well-behaved ones DELETE FROM FLOAT8_TBL;
=========================================
This happening on cygwin 1.5.20 (running on top of winXP), gcc 3.4.4.
The entire check.log can be found here : http://www.newsoftcontrol.ro/~am/pgfarm/check.log
The other logs generated by the buildfarm can be found here: http://www.newsoftcontrol.ro/~am/pgfarm/
Cheers,
Adrian Maier
Adrian Maier schrieb:
> Hello,
>
> While setting up a buildfarm installation for cygwin, I've
> uncountered the following
> regression failure :
>
> float8 ... FAILED
>
> ================== pgsql.3132/src/test/regress/regression.diffs
> *** ./expected/float8-small-is-zero.out Tue Jul 18 09:24:52 2006
> --- ./results/float8.out Tue Jul 18 09:53:42 2006
> ***************
> *** 13,29 ****
> SELECT '-10e400'::float8;
> ERROR: "-10e400" is out of range for type double precision
> SELECT '10e-400'::float8;
> ! float8
> ! --------
> ! 0
> ! (1 row)
> !
> SELECT '-10e-400'::float8;
> ! float8
> ! --------
> ! -0
> ! (1 row)
> !
> -- bad input
> INSERT INTO FLOAT8_TBL(f1) VALUES ('');
> ERROR: invalid input syntax for type double precision: ""
> --- 13,21 ----
> SELECT '-10e400'::float8;
> ERROR: "-10e400" is out of range for type double precision
> SELECT '10e-400'::float8;
> ! ERROR: "10e-400" is out of range for type double precision
> SELECT '-10e-400'::float8;
> ! ERROR: "-10e-400" is out of range for type double precision
> -- bad input
> INSERT INTO FLOAT8_TBL(f1) VALUES ('');
> ERROR: invalid input syntax for type double precision: ""
> ***************
> *** 377,383 ****
> --- 369,377 ----
> INSERT INTO FLOAT8_TBL(f1) VALUES ('-10e400');
> ERROR: "-10e400" is out of range for type double precision
> INSERT INTO FLOAT8_TBL(f1) VALUES ('10e-400');
> + ERROR: "10e-400" is out of range for type double precision
> INSERT INTO FLOAT8_TBL(f1) VALUES ('-10e-400');
> + ERROR: "-10e-400" is out of range for type double precision
> -- maintain external table consistency across platforms
> -- delete all values and reinsert well-behaved ones
> DELETE FROM FLOAT8_TBL;
> =========================================
>
> This happening on cygwin 1.5.20 (running on top of winXP), gcc 3.4.4.
>
>
> The entire check.log can be found here :
> http://www.newsoftcontrol.ro/~am/pgfarm/check.log
> The other logs generated by the buildfarm can be found here:
> http://www.newsoftcontrol.ro/~am/pgfarm/
Thanks,
Which postgresql version?
Can we have a regular cygwin error report please mailed to cygwin at
cygwin.com please. See http://cygwin.com/problems.html (cygcheck -s -v
-r > cygcheck.out)
Looks like a strtod() newlib feature, but I haven't inspected closely.
http://sources.redhat.com/ml/newlib/2006/msg00020.html
BTW: HAVE_LONG_LONG_INT_64 is defined, so INT64_IS_BUSTED is defined also.
Does it look the same on redhat fedora?
Our buildfarm doesn't have these issues,
this runs gcc-3.3.3 and gcc-3.4.4
The problem I see is that float8in() in
src/backend/utils/adt/float.c checks only for "-Infinity" and not "-inf"
as returned by newlib: pg_strncasecmp(num, "-Infinity", 9) == 0
Can you test this?
$ cat test-strtod.c
#include <stdlib.h>
#include <stdio.h>
#include <errno.h>
#include <float.h>
double d;
char *tail;
int main() { errno = 0; d = strtod("-10e400", &tail); printf("double: %f, errno: %d, tail: '%s', isinf: %d,
fabs:%f,
inf>max: %d" , d, errno, tail, isinf(d), fabs(d), fabs(d) > DBL_MAX ? 1 : 0);
}
$ gcc test-strtod.c; ./a
double: -inf, errno: 34, tail: '', isinf: 1, fabs: inf, inf>max: 1
--
Reini Urban - postgresql-cygwin maintainer
http://phpwiki.org/ http://murbreak.at/
http://helsinki.at/ http://spacemovie.mur.at/
On 20/07/06, Reini Urban <rurban@x-ray.at> wrote: > > Thanks, > Which postgresql version? The version is cvs HEAD. > Can we have a regular cygwin error report please mailed to cygwin at > cygwin.com please. See http://cygwin.com/problems.html (cygcheck -s -v > -r > cygcheck.out) > > Looks like a strtod() newlib feature, but I haven't inspected closely. > http://sources.redhat.com/ml/newlib/2006/msg00020.html > > BTW: HAVE_LONG_LONG_INT_64 is defined, so INT64_IS_BUSTED is defined also. > > Does it look the same on redhat fedora? > Our buildfarm doesn't have these issues, this runs gcc-3.3.3 and gcc-3.4.4 I'm not sure about fedora, but on NetBSD 3.0rc5 , postgresql 8.1.3 I can see the same behaviour: am=# select version(); version --------------------------------------------------------------------------------------------PostgreSQL 8.1.3 on i386--netbsdelf,compiled by GCC gcc (GCC) 3.3.3 (NetBSD nb3 20040520) (1 row) am=# SELECT '-10e400'::float8; ERROR: "-10e400" is out of range for type double precision > The problem I see is that float8in() in > src/backend/utils/adt/float.c checks only for "-Infinity" and not "-inf" > as returned by newlib: > pg_strncasecmp(num, "-Infinity", 9) == 0 > > Can you test this? Sure . > $ gcc test-strtod.c; ./a > double: -inf, errno: 34, tail: '', isinf: 1, fabs: inf, inf>max: 1 Yes, I'm getting the same output (both on cygwin and netbsd 3.0rc5): $ ./test-strtod.exe double: -inf, errno: 34, tail: '', isinf: 1, fabs: inf,inf>max: 1 Cheers, Adrian Maier
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?
regards, tom lane
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
Attachment
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: > >> 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? > > > Yes, good points. One other thought I had was that we could have pg_regress always allow a fallback to the canonical result file. So in this case it would try float8-small-is-zero.out and float8-small-is-zero_1.out and finally float8.out (the first would succeed on eel, the last on cassowary, I am speculating). In general this would allow a configuration to become more correct painlessly. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> 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.
> Yes, good points. 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?
One thing I realized while looking at resultmap just now is that the
handmade regex matcher code I wrote for pg_regress.c (lifted from the
backend's LIKE algorithm) doesn't handle bracketed character classes,
but we've got one of those in resultmap:
float8/i.86-.*-freebsd[234]=float8-small-is-zero
Dunno how I missed that :-(. So presumably HEAD would be failing right
now on freebsd-before-5.0, if we had any of 'em in the buildfarm.
Adding char-class code to the matcher wouldn't be too hard, but it
probably wouldn't solve the cygwin problem, and as I mentioned I feel
zero allegiance to the historical format of resultmap anyway --- it
was designed the way it is purely because it was fairly easy to handle
in a few lines of shell script.
If we used your idea of always allowing a fallback to the canonical
file, then we could just make the freebsd line
float8/i.86-.*-freebsd=float8-small-is-zero
and it would do the right thing. Alternatively we could do something
more adventurous with matching to OS versions --- I'm envisioning
something roughly like
float8/i.86-.*-freebsd<5=float8-small-is-zerofloat8/i.86-pc-cygwin<5.0.21=float8-small-is-zero
This would be more flexible in the long run, but might be overkill.
The fallback idea sounds like a reasonable compromise ...
regards, tom lane
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
On 01/08/06, Andrew Dunstan <andrew@dunslane.net> wrote: > 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. The cassowary farm member went green, for the first time in history :) Thanks guys, Adrian Maier