SEO

Tom Lane wrote:
> 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...);

I wasn't excited about doing one isinf() call to avoid three, so I just
made a fast isinf() macro:

/* We call isinf() a lot, so we use a fast version in this file */
#define fast_isinf(val) (((val) < DBL_MIN || (val) > DBL_MAX) && isinf(val))

and used that instead of the direct isinf() call. (We do call fabs() in
the Check* routines. Should we be using our own Abs()?) The new patch
also uses float8 for float4 computations, and adds a comment about why
(avoid underflow in some cases).

In looking at the idea of checking for zero as an underflow, I found
most transcendental functions already had such a check, so I moved the check
into the Check*() routines, and added checks for multiplication/division
underflow to zero. The only outstanding uncaught underflow is from
addition/subtraction.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Bruce Momjian <bruce@momjian.us> writes:
> I wasn't excited about doing one isinf() call to avoid three, so I just
> made a fast isinf() macro:

> /* We call isinf() a lot, so we use a fast version in this file */
> #define fast_isinf(val) (((val) < DBL_MIN || (val) > DBL_MAX) && isinf(val))

This is *not* going in the right direction :-(

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I wasn't excited about doing one isinf() call to avoid three, so I just
> > made a fast isinf() macro:
>
> > /* We call isinf() a lot, so we use a fast version in this file */
> > #define fast_isinf(val) (((val) < DBL_MIN || (val) > DBL_MAX) && isinf(val))
>
> This is *not* going in the right direction :-(

Well, then show me what direction you think is better.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> This is *not* going in the right direction :-(

> Well, then show me what direction you think is better.

Fewer restrictions, not more. The thrust of what I've been saying
(and I think Roman too) is to trust in the hardware float-arithmetic
implementation to be right. Every time you add an additional "error
check" you are going in the wrong direction.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> This is *not* going in the right direction :-(
>
> > Well, then show me what direction you think is better.
>
> Fewer restrictions, not more. The thrust of what I've been saying
> (and I think Roman too) is to trust in the hardware float-arithmetic
> implementation to be right. Every time you add an additional "error
> check" you are going in the wrong direction.

OK, are you saying that there is a signal we are ignoring for
overflow/underflow, or that we should just silently overflow/underflow
and not throw an error?

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Bruce Momjian <bruce@momjian.us> writes:
> OK, are you saying that there is a signal we are ignoring for
> overflow/underflow, or that we should just silently overflow/underflow
> and not throw an error?

Silent underflow is fine with me; it's the norm in most all float
implementations and won't surprise anyone. For overflow I'm OK with
either returning infinity or throwing an error --- but if an error,
it should only be about inf-out-with-non-inf-in, not comparisons to any
artificial MAX/MIN values.

Anyone else have an opinion about this?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
>> OK, are you saying that there is a signal we are ignoring for
>> overflow/underflow, or that we should just silently overflow/underflow
>> and not throw an error?
>
> Silent underflow is fine with me; it's the norm in most all float
> implementations and won't surprise anyone. For overflow I'm OK with
> either returning infinity or throwing an error --- but if an error,
> it should only be about inf-out-with-non-inf-in, not comparisons to any
> artificial MAX/MIN values.
>
> Anyone else have an opinion about this?
If an underflow is not reported (And thus silently treated as zero), then
it'd make sense for me to deal with overflows in a similar way, and just
return infinity.

The most correct solution would IMHO be to provide a guc variable
"strict_float_semantics" that defaults to "off", meaning that neather
overflow nor underflow reports an error. If the variable was set to on,
_both_ overflow and underflow would be reported.

Just my €0.02

greetings, Florian Pflug




---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

On 12/29/2006 12:23 AM, Bruce Momjian wrote:
> Well, then show me what direction you think is better.

Think about this idea please. This has no INF, NaN or range
checks and detects all "bad" cases with any floating point
math.

The only issue is that a bad case is detected only once.
You need to restart the postmaster. It can be fixed by
re-enabling FP exceptions in the FP exception handler.

Roman
-----------------------------
~/postgresql-8.2.0/src/backend/utils/adt>diff -U3 -p float.orig.c float.c
--- float.orig.c 2006-12-29 10:49:51.000000000 -0600
+++ float.c 2006-12-29 10:58:19.000000000 -0600
@@ -60,12 +60,21 @@
#ifdef HAVE_IEEEFP_H
#include <ieeefp.h>
#endif
+#include <fenv.h>

#include "catalog/pg_type.h"
#include "libpq/pqformat.h"
#include "utils/array.h"
#include "utils/builtins.h"

+static void __attribute__((__constructor__))
+enable_fp_exceptions()
+{
+ feclearexcept(FE_ALL_EXCEPT);
+ feenableexcept(FE_DIVBYZERO | FE_UNDERFLOW | FE_OVERFLOW | FE_INVALID);
+ printf("FP exceptions enabled\n");
+}
+

#ifndef M_PI
/* from my RH5.2 gcc math.h file - thomas 2000-04-03 */
@@ -783,11 +792,10 @@ float4pl(PG_FUNCTION_ARGS)
{
float4 arg1 = PG_GETARG_FLOAT4(0);
float4 arg2 = PG_GETARG_FLOAT4(1);
- double result;
+ float4 result;

result = arg1 + arg2;
- CheckFloat4Val(result);
- PG_RETURN_FLOAT4((float4) result);
+ PG_RETURN_FLOAT4(result);
}

Datum
@@ -795,11 +803,10 @@ float4mi(PG_FUNCTION_ARGS)
{
float4 arg1 = PG_GETARG_FLOAT4(0);
float4 arg2 = PG_GETARG_FLOAT4(1);
- double result;
+ float4 result;

result = arg1 - arg2;
- CheckFloat4Val(result);
- PG_RETURN_FLOAT4((float4) result);
+ PG_RETURN_FLOAT4(result);
}

Datum
@@ -807,11 +814,10 @@ float4mul(PG_FUNCTION_ARGS)
{
float4 arg1 = PG_GETARG_FLOAT4(0);
float4 arg2 = PG_GETARG_FLOAT4(1);
- double result;
+ float4 result;

result = arg1 * arg2;
- CheckFloat4Val(result);
- PG_RETURN_FLOAT4((float4) result);
+ PG_RETURN_FLOAT4(result);
}

Datum
@@ -819,18 +825,10 @@ float4div(PG_FUNCTION_ARGS)
{
float4 arg1 = PG_GETARG_FLOAT4(0);
float4 arg2 = PG_GETARG_FLOAT4(1);
- double result;
-
- if (arg2 == 0.0)
- ereport(ERROR,
- (errcode(ERRCODE_DIVISION_BY_ZERO),
- errmsg("division by zero")));
-
- /* Do division in float8, then check for overflow */
- result = (float8) arg1 / (float8) arg2;
+ float4 result;

- CheckFloat4Val(result);
- PG_RETURN_FLOAT4((float4) result);
+ result = arg1 / arg2;
+ PG_RETURN_FLOAT4(result);
}

/*


---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

On 12/29/2006 11:27 AM, Tom Lane wrote:
> Doesn't even compile here (no <fenv.h>).

Where do you compile?

Roman


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > OK, are you saying that there is a signal we are ignoring for
> > overflow/underflow, or that we should just silently overflow/underflow
> > and not throw an error?
>
> Silent underflow is fine with me; it's the norm in most all float
> implementations and won't surprise anyone. For overflow I'm OK with
> either returning infinity or throwing an error --- but if an error,
> it should only be about inf-out-with-non-inf-in, not comparisons to any
> artificial MAX/MIN values.

OK, I am happy to remove the MIN/MAX comparisons. Those were in the
original code.

The attached, updated patch creates a single CHECKFLOATVAL() macro that
does the overflow/underflow comparisons and throws an error. This also
reduces the isinf() calls. Should I be concerned we are now duplicating
the error text in all call sites?

Regression wording modified now that float4/float8 checks are merged. I
haven't update the platform-specific float* expected files yet, but will
on commit.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster