NaN divided by zero should yield NaN

Started by Tom Laneover 5 years ago3 messages

tgl@sss.pgh.pa.us

over 5 years ago

1 attachment(s)

Dean Rasheed questioned this longstanding behavior:

regression=# SELECT 'nan'::float8 / '0'::float8;
ERROR: division by zero

After a bit of research I think he's right: per IEEE 754 this should
yield NaN, not an error. Accordingly I propose the attached patch.
This is probably not something to back-patch, though.

One thing that's not very clear to me is which of these spellings
is preferable:

if (unlikely(val2 == 0.0) && !isnan(val1))
if (unlikely(val2 == 0.0 && !isnan(val1)))

I think we can reject this variant:

if (unlikely(val2 == 0.0) && unlikely(!isnan(val1)))

since actually the second condition *is* pretty likely.
But I don't know which of the first two would give better
code. Andres, any thoughts?

regards, tom lane

Attachments:

nan-over-zero-is-nan.patchtext/x-diff; charset=us-ascii; name=nan-over-zero-is-nan.patchDownload

diff --git a/src/include/utils/float.h b/src/include/utils/float.h
index 8d4bbc51a6..e2aae8ef17 100644
--- a/src/include/utils/float.h
+++ b/src/include/utils/float.h
@@ -222,7 +222,7 @@ float4_div(const float4 val1, const float4 val2)
 {
 	float4		result;
 
-	if (unlikely(val2 == 0.0f))
+	if (unlikely(val2 == 0.0f) && !isnan(val1))
 		float_zero_divide_error();
 	result = val1 / val2;
 	if (unlikely(isinf(result)) && !isinf(val1) && !isinf(val2))
@@ -238,7 +238,7 @@ float8_div(const float8 val1, const float8 val2)
 {
 	float8		result;
 
-	if (unlikely(val2 == 0.0))
+	if (unlikely(val2 == 0.0) && !isnan(val1))
 		float_zero_divide_error();
 	result = val1 / val2;
 	if (unlikely(isinf(result)) && !isinf(val1) && !isinf(val2))
diff --git a/src/test/regress/expected/float4-misrounded-input.out b/src/test/regress/expected/float4-misrounded-input.out
index 6c89af6394..38b847a1ad 100644
--- a/src/test/regress/expected/float4-misrounded-input.out
+++ b/src/test/regress/expected/float4-misrounded-input.out
@@ -143,6 +143,12 @@ SELECT 'nan'::float4 / 'nan'::float4;
       NaN
 (1 row)
 
+SELECT 'nan'::float4 / '0'::float4;
+ ?column? 
+----------
+      NaN
+(1 row)
+
 SELECT 'nan'::numeric::float4;
  float4 
 --------
diff --git a/src/test/regress/expected/float4.out b/src/test/regress/expected/float4.out
index d6c22c1752..310402b6ee 100644
--- a/src/test/regress/expected/float4.out
+++ b/src/test/regress/expected/float4.out
@@ -143,6 +143,12 @@ SELECT 'nan'::float4 / 'nan'::float4;
       NaN
 (1 row)
 
+SELECT 'nan'::float4 / '0'::float4;
+ ?column? 
+----------
+      NaN
+(1 row)
+
 SELECT 'nan'::numeric::float4;
  float4 
 --------
diff --git a/src/test/regress/expected/float8.out b/src/test/regress/expected/float8.out
index c635dd7dcb..2304b579d2 100644
--- a/src/test/regress/expected/float8.out
+++ b/src/test/regress/expected/float8.out
@@ -126,6 +126,12 @@ SELECT 'nan'::float8 / 'nan'::float8;
       NaN
 (1 row)
 
+SELECT 'nan'::float8 / '0'::float8;
+ ?column? 
+----------
+      NaN
+(1 row)
+
 SELECT 'nan'::numeric::float8;
  float8 
 --------
diff --git a/src/test/regress/sql/float4.sql b/src/test/regress/sql/float4.sql
index 393d98fb14..1fcf823c21 100644
--- a/src/test/regress/sql/float4.sql
+++ b/src/test/regress/sql/float4.sql
@@ -50,6 +50,7 @@ SELECT ' INFINITY    x'::float4;
 SELECT 'Infinity'::float4 + 100.0;
 SELECT 'Infinity'::float4 / 'Infinity'::float4;
 SELECT 'nan'::float4 / 'nan'::float4;
+SELECT 'nan'::float4 / '0'::float4;
 SELECT 'nan'::numeric::float4;
 
 SELECT '' AS five, * FROM FLOAT4_TBL;
diff --git a/src/test/regress/sql/float8.sql b/src/test/regress/sql/float8.sql
index 288969aed6..f103871cdb 100644
--- a/src/test/regress/sql/float8.sql
+++ b/src/test/regress/sql/float8.sql
@@ -43,6 +43,7 @@ SELECT ' INFINITY    x'::float8;
 SELECT 'Infinity'::float8 + 100.0;
 SELECT 'Infinity'::float8 / 'Infinity'::float8;
 SELECT 'nan'::float8 / 'nan'::float8;
+SELECT 'nan'::float8 / '0'::float8;
 SELECT 'nan'::numeric::float8;
 
 SELECT '' AS five, * FROM FLOAT8_TBL;

Dean Rasheed

dean.a.rasheed@gmail.com

over 5 years ago

In reply to: Tom Lane (#1)

Re: NaN divided by zero should yield NaN

On Thu, 16 Jul 2020 at 20:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dean Rasheed questioned this longstanding behavior:

regression=# SELECT 'nan'::float8 / '0'::float8;
ERROR: division by zero

After a bit of research I think he's right: per IEEE 754 this should
yield NaN, not an error. Accordingly I propose the attached patch.
This is probably not something to back-patch, though.

Agreed.

One thing that's not very clear to me is which of these spellings
is preferable:

if (unlikely(val2 == 0.0) && !isnan(val1))
if (unlikely(val2 == 0.0 && !isnan(val1)))

My guess is that the first would be better, since it would tell the
compiler that it's unlikely to need to do the NaN test, so it would be
kind of like doing

if (unlikely(val2 == 0.0))
if (!isnan(val1)))

Regards,
Dean

Tom Lane

tgl@sss.pgh.pa.us

over 5 years ago

In reply to: Dean Rasheed (#2)

Re: NaN divided by zero should yield NaN

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

On Thu, 16 Jul 2020 at 20:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:

One thing that's not very clear to me is which of these spellings
is preferable:
if (unlikely(val2 == 0.0) && !isnan(val1))
if (unlikely(val2 == 0.0 && !isnan(val1)))

My guess is that the first would be better, since it would tell the
compiler that it's unlikely to need to do the NaN test,

Yeah, that's the straightforward way to think about it, but I've
found that gcc is sometimes less than straightforward ;-). Still,
there's no obvious reason to do it the second way, so I pushed the
first way.

regards, tom lane