Thread: Re: [PATCHES] [BUGS] BUG #2846: inconsistent and confusing handling of
Bruce Momjian <bruce@momjian.us> writes:
> I have made some more progress on this patch.
I'm not convinced that you're fixing things so much as doing your best
to destroy IEEE-compliant float arithmetic behavior.
I think what we should probably consider is removing CheckFloat4Val
and CheckFloat8Val altogether, and just letting the float arithmetic
have its head. Most modern hardware gets float arithmetic right per
spec, and we shouldn't be second-guessing it.
A slightly less radical proposal is to reject only the case where
isinf(result) and neither input isinf(); and perhaps likewise with
respect to NaNs.
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I have made some more progress on this patch.
>
> I'm not convinced that you're fixing things so much as doing your best
> to destroy IEEE-compliant float arithmetic behavior.
>
> I think what we should probably consider is removing CheckFloat4Val
> and CheckFloat8Val altogether, and just letting the float arithmetic
> have its head. Most modern hardware gets float arithmetic right per
> spec, and we shouldn't be second-guessing it.
Well, I am on an Xeon and can confirm that our computations of large
non-infinite doubles who's result greatly exceed the max double are
indeed returning infinity, as the poster reported, so something isn't
working, if it supposed to. What do people get for this computation?
#include <stdio.h>
#include <stdlib.h>
int
main(int argc, char *argv[])
{
double a = 1e300, b = 1e300;
double c;
c = a * b;
printf("%e\n", c);
return 0;
}
I get 'inf'. I am on BSD and just tested it on Fedora Core 2 and got
'inf' too.
> A slightly less radical proposal is to reject only the case where
> isinf(result) and neither input isinf(); and perhaps likewise with
> respect to NaNs.
Uh, that's what the patch does for 'Inf':
result = arg1 + arg2;
CheckFloat4Val(result, isinf(arg1) || isinf(arg2));
I didn't touch 'Nan' because that is passed around as a value just fine
--- it isn't created or tested as part of an overflow.
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
> I get 'inf'. I am on BSD and just tested it on Fedora Core 2 and got
> 'inf' too.
Ubuntu Edgy 64bit on Athlon 64X2 returns inf.
Joshua D. Drake
>
> > A slightly less radical proposal is to reject only the case where
> > isinf(result) and neither input isinf(); and perhaps likewise with
> > respect to NaNs.
>
> Uh, that's what the patch does for 'Inf':
>
> result = arg1 + arg2;
> CheckFloat4Val(result, isinf(arg1) || isinf(arg2));
>
> I didn't touch 'Nan' because that is passed around as a value just fine
> --- it isn't created or tested as part of an overflow.
>
--
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> I think what we should probably consider is removing CheckFloat4Val
>> and CheckFloat8Val altogether, and just letting the float arithmetic
>> have its head. Most modern hardware gets float arithmetic right per
>> spec, and we shouldn't be second-guessing it.
> Well, I am on an Xeon and can confirm that our computations of large
> non-infinite doubles who's result greatly exceed the max double are
> indeed returning infinity, as the poster reported, so something isn't
> working, if it supposed to. What do people get for this computation?
Infinity is what you are *supposed* to get, per IEEE spec.
>> A slightly less radical proposal is to reject only the case where
>> isinf(result) and neither input isinf(); and perhaps likewise with
>> respect to NaNs.
> Uh, that's what the patch does for 'Inf':
> result = arg1 + arg2;
> CheckFloat4Val(result, isinf(arg1) || isinf(arg2));
No, because you are still comparing against FLOAT4_MAX. I'm suggesting
that only an actual infinity should be rejected. Even that is contrary
to IEEE spec, though.
The other problem with this coding technique is that it must invoke
isinf three times when the typical case really only requires one (if the
output isn't inf there is no need to perform isinf on the inputs).
If we're going to check for overflow at all, I think we should lose the
subroutine and just do
if (isinf(result) &&
!(isinf(arg1) || isinf(arg2)))
ereport(...OVERFLOW...);
regards, tom lane
On 12/27/2006 01:15 PM, Tom Lane wrote: > I'm not convinced that you're fixing things so much as doing your best > to destroy IEEE-compliant float arithmetic behavior. > > I think what we should probably consider is removing CheckFloat4Val > and CheckFloat8Val altogether, and just letting the float arithmetic > have its head. Most modern hardware gets float arithmetic right per > spec, and we shouldn't be second-guessing it. I vote for complete IEEE-compliance. No exceptions with pure floating point math. Float -> int conversions should reject overflow, INF and NaN. Float -> numeric conversions should reject INF. > A slightly less radical proposal is to reject only the case where > isinf(result) and neither input isinf(); and perhaps likewise with > respect to NaNs. This might look like another possibility for confusion. For example INF-INF=NaN. Regards, Roman.
Roman Kononov wrote: > On 12/27/2006 01:15 PM, Tom Lane wrote: > > I'm not convinced that you're fixing things so much as doing your best > > to destroy IEEE-compliant float arithmetic behavior. > > > > I think what we should probably consider is removing CheckFloat4Val > > and CheckFloat8Val altogether, and just letting the float arithmetic > > have its head. Most modern hardware gets float arithmetic right per > > spec, and we shouldn't be second-guessing it. > > I vote for complete IEEE-compliance. No exceptions with pure floating > point math. Float -> int conversions should reject overflow, INF and > NaN. Float -> numeric conversions should reject INF. I think we did that in current CVS. Feel free to download a snapshot from the ftp server and try it out. > > A slightly less radical proposal is to reject only the case where > > isinf(result) and neither input isinf(); and perhaps likewise with > > respect to NaNs. > > This might look like another possibility for confusion. For example > INF-INF=NaN. Yep, that's what we get now. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +