power() function in Windows: "value out of range: underflow"
Hi,
There are some cases that power() function does not work
correctly with 'NaN' arguments in Windows environment.
Something like,
postgres=# select power('NaN',11);
ERROR: value out of range: underflow
postgres=# select power('NaN','NaN');
ERROR: value out of range: underflow
postgres=# select power(11,'NaN');
ERROR: value out of range: underflow
In Linux environment, instead of ERROR it returns 'NaN'.
The reason here is,
When pow() in float.c:dpow() is called with 'NaN' arguments,
pow() returns 'NaN' but in Windows environment errno is set to
EDOM(invalid floating-point exception).
So, PostgreSQL update "result" and cause an ERROR in CHECKFLOATVAL macro.
I think it should be return 'NaN' in all of above cases.
I have tried to create a patch to fix it.
Please confirm the attached file.
---
Thanks and best regards,
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/
Attachments:
power_NaN.PATCHapplication/octet-stream; name=power_NaN.PATCHDownload
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 6522c08..99725bf 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1573,7 +1573,7 @@ dpow(PG_FUNCTION_ARGS)
*/
errno = 0;
result = pow(arg1, arg2);
- if (errno == EDOM && isnan(result))
+ if (errno == EDOM && isnan(result) && !(isnan(arg1) || isnan(arg2)))
{
if ((fabs(arg1) > 1 && arg2 >= 0) || (fabs(arg1) < 1 && arg2 < 0))
/* The sign of Inf is not significant in this case. */
diff --git a/src/test/regress/expected/float8.out b/src/test/regress/expected/float8.out
index 20c985e..6d7cdc3 100644
--- a/src/test/regress/expected/float8.out
+++ b/src/test/regress/expected/float8.out
@@ -340,6 +340,24 @@ SELECT power(float8 '144', float8 '0.5');
12
(1 row)
+SELECT power(float8 'NaN', float8 '0.5');
+ power
+-------
+ NaN
+(1 row)
+
+SELECT power(float8 '144', float8 'NaN');
+ power
+-------
+ NaN
+(1 row)
+
+SELECT power(float8 'NaN', float8 'NaN');
+ power
+-------
+ NaN
+(1 row)
+
-- take exp of ln(f.f1)
SELECT '' AS three, f.f1, exp(ln(f.f1)) AS exp_ln_f1
FROM FLOAT8_TBL f
diff --git a/src/test/regress/sql/float8.sql b/src/test/regress/sql/float8.sql
index 215e7a4..dca90f7 100644
--- a/src/test/regress/sql/float8.sql
+++ b/src/test/regress/sql/float8.sql
@@ -108,6 +108,9 @@ SELECT '' AS three, f.f1, |/f.f1 AS sqrt_f1
-- power
SELECT power(float8 '144', float8 '0.5');
+SELECT power(float8 'NaN', float8 '0.5');
+SELECT power(float8 '144', float8 'NaN');
+SELECT power(float8 'NaN', float8 'NaN');
-- take exp of ln(f.f1)
SELECT '' AS three, f.f1, exp(ln(f.f1)) AS exp_ln_f1
2018-04-10 5:30 GMT-03:00 Huong Dangminh <huo-dangminh@ys.jp.nec.com>:
There are some cases that power() function does not work
correctly with 'NaN' arguments in Windows environment.
Something like,
What is your exact OS version? What is your postgres version? I tested
with Windows 10 (10.0.16299) x64 and couldn't reproduce the error.
postgres=# select power('NaN',11);
ERROR: value out of range: underflow
postgres=# select power('NaN','NaN');
ERROR: value out of range: underflow
postgres=# select power(11,'NaN');
ERROR: value out of range: underflowIn Linux environment, instead of ERROR it returns 'NaN'.
Could you show us a simple test case? Print arguments, result and errno.
--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Hi,
Thanks for response
# I will add this thread to current CF soon.
2018-04-10 5:30 GMT-03:00 Huong Dangminh <huo-dangminh@ys.jp.nec.com>:
There are some cases that power() function does not work correctly
with 'NaN' arguments in Windows environment.
Something like,What is your exact OS version? What is your postgres version? I tested with
Windows 10 (10.0.16299) x64 and couldn't reproduce the error.
I think it is not depended on OS version but the visual c++ runtime libraries version
(It seem also be included in installers).
My environment:
====================================
- PostgreSQL: 9.6.8
# I am using the EDB PostgreSQL Installer
# It also can reproduce in PostgreSQL 10.3 or 9.5.12
postgres -V
postgres (PostgreSQL) 9.6.8
psql -c "select version()"
version
-------------------------------------------------------------
PostgreSQL 9.6.8, compiled by Visual C++ build 1800, 64-bit
(1 row)
psql -c "select 'NaN'^11"
ERROR: value out of range: underflow
- OS: Microsoft Windows 10 Pro 10.0.14393 build 14393
=============================
postgres=# select power('NaN',11);
ERROR: value out of range: underflow
postgres=# select power('NaN','NaN');
ERROR: value out of range: underflow
postgres=# select power(11,'NaN');
ERROR: value out of range: underflowIn Linux environment, instead of ERROR it returns 'NaN'.
Could you show us a simple test case? Print arguments, result and errno.
It just is my confirmation when debug PostgreSQL in Windows environment.
# I built PostgreSQL with VC++ 2012 in debug mode and confirmed.
I don't know how to print those values.
---
Thanks and best regards,
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/
On 10 April 2018 at 20:30, Huong Dangminh <huo-dangminh@ys.jp.nec.com> wrote:
Hi,
There are some cases that power() function does not work
correctly with 'NaN' arguments in Windows environment.
Something like,postgres=# select power('NaN',11);
ERROR: value out of range: underflow
postgres=# select power('NaN','NaN');
ERROR: value out of range: underflow
postgres=# select power(11,'NaN');
ERROR: value out of range: underflowIn Linux environment, instead of ERROR it returns 'NaN'.
The reason here is,
When pow() in float.c:dpow() is called with 'NaN' arguments,
pow() returns 'NaN' but in Windows environment errno is set to
EDOM(invalid floating-point exception).
So, PostgreSQL update "result" and cause an ERROR in CHECKFLOATVAL macro.
I can recreate this when building with MSVC 2012. I confirm that I see
the same as you. Microsoft are setting errno to EDOM in the above 3
cases, where in Linux the result is still NaN, just the errno is not
set.
Looking at [1]http://pubs.opengroup.org/onlinepubs/9699919799/functions/pow.html, it says:
* If x or y is a NaN, a NaN shall be returned (unless specified
elsewhere in this description).
* For any value of y (including NaN), if x is +1, 1.0 shall be returned.
* For any value of x (including NaN), if y is ±0, 1.0 shall be returned.
Which Microsoft seem to not have broken, they're just also setting the
errno to EDOM in the first case, which seems a little strange, but the
standard there does not seem to mention that it shouldn't do that.
You patch fixes the problem for me, and importantly the two following
cases still work:
postgres=# select power(1,'NaN');
power
-------
1
(1 row)
postgres=# select power('NaN', 0);
power
-------
1
(1 row)
There's no mention in the SQL standard about NaN handling in the above
two cases, but I wonder if it's worth also adding a couple of tests
for the above two cases to ensure all platforms are doing the same
thing... ?
I'd also rather see this line:
if (errno == EDOM && isnan(result) && !(isnan(arg1) || isnan(arg2)))
written as:
if (errno == EDOM && isnan(result) && !isnan(arg1) && !isnan(arg2))
[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/pow.html
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
2018-04-11 0:13 GMT-03:00 David Rowley <david.rowley@2ndquadrant.com>:
I can recreate this when building with MSVC 2012. I confirm that I see
the same as you. Microsoft are setting errno to EDOM in the above 3
cases, where in Linux the result is still NaN, just the errno is not
set.
FWIW, I tested in MSVC 2017 (15.6.4) and it worked like expected.
Looking at [1]https://docs.microsoft.com/en-us/cpp/porting/floating-point-migration-issues, it seems there could be nasty bugs when using math
functions in MSVC < 2013 (or 2015). I don't have some older MSVC here
to try /fp:OPTIONHERE [2]https://msdn.microsoft.com/pt-br/library/e7s85ffb.aspx to see if it makes any difference.
[1]: https://docs.microsoft.com/en-us/cpp/porting/floating-point-migration-issues
[2]: https://msdn.microsoft.com/pt-br/library/e7s85ffb.aspx
--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Thanks for confirming.
2018-04-11 0:13 GMT-03:00 David Rowley <david.rowley@2ndquadrant.com>:
I can recreate this when building with MSVC 2012. I confirm that I see
the same as you. Microsoft are setting errno to EDOM in the above 3
cases, where in Linux the result is still NaN, just the errno is not
set.FWIW, I tested in MSVC 2017 (15.6.4) and it worked like expected.
Looking at [1], it seems there could be nasty bugs when using math functions
in MSVC < 2013 (or 2015). I don't have some older MSVC here to try
/fp:OPTIONHERE [2] to see if it makes any difference.
Thanks.
Anyway currently PostgreSQL installers seem not be compiled with MSVC 2017,
so it should be fix for current users?
I updated the patch as David Rowley mentioned.
---
Thanks and best regards,
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/
Attachments:
dpow_NaN_V2.patchapplication/octet-stream; name=dpow_NaN_V2.patchDownload
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 6522c08..1483d9f 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1570,10 +1570,14 @@ dpow(PG_FUNCTION_ARGS)
* using something different from our floor() test to decide it's
* invalid). Other platforms (HPPA) return errno == ERANGE and a large
* (HUGE_VAL) but finite result to signal overflow.
+ *
+ * Note that some versions of Visual Studio set errno to EDOM when
+ * arg1 or arg2 is a NaN.
+ * So we need to except those cases here.
*/
errno = 0;
result = pow(arg1, arg2);
- if (errno == EDOM && isnan(result))
+ if (errno == EDOM && isnan(result) && !isnan(arg1) && !isnan(arg2))
{
if ((fabs(arg1) > 1 && arg2 >= 0) || (fabs(arg1) < 1 && arg2 < 0))
/* The sign of Inf is not significant in this case. */
diff --git a/src/test/regress/expected/float8.out b/src/test/regress/expected/float8.out
index 20c985e..ede9aaf 100644
--- a/src/test/regress/expected/float8.out
+++ b/src/test/regress/expected/float8.out
@@ -340,6 +340,36 @@ SELECT power(float8 '144', float8 '0.5');
12
(1 row)
+SELECT power(float8 'NaN', float8 '0.5');
+ power
+-------
+ NaN
+(1 row)
+
+SELECT power(float8 '144', float8 'NaN');
+ power
+-------
+ NaN
+(1 row)
+
+SELECT power(float8 'NaN', float8 'NaN');
+ power
+-------
+ NaN
+(1 row)
+
+SELECT power(float8 '1',float8 'NaN');
+ power
+-------
+ 1
+(1 row)
+
+SELECT power(float8 'NaN',float8 '0');
+ power
+-------
+ 1
+(1 row)
+
-- take exp of ln(f.f1)
SELECT '' AS three, f.f1, exp(ln(f.f1)) AS exp_ln_f1
FROM FLOAT8_TBL f
diff --git a/src/test/regress/sql/float8.sql b/src/test/regress/sql/float8.sql
index 215e7a4..9d1d607 100644
--- a/src/test/regress/sql/float8.sql
+++ b/src/test/regress/sql/float8.sql
@@ -108,6 +108,11 @@ SELECT '' AS three, f.f1, |/f.f1 AS sqrt_f1
-- power
SELECT power(float8 '144', float8 '0.5');
+SELECT power(float8 'NaN', float8 '0.5');
+SELECT power(float8 '144', float8 'NaN');
+SELECT power(float8 'NaN', float8 'NaN');
+SELECT power(float8 '1',float8 'NaN');
+SELECT power(float8 'NaN',float8 '0');
-- take exp of ln(f.f1)
SELECT '' AS three, f.f1, exp(ln(f.f1)) AS exp_ln_f1
On 11 April 2018 at 16:42, Huong Dangminh <huo-dangminh@ys.jp.nec.com> wrote:
I updated the patch as David Rowley mentioned.
Looks fine to me. Please add to the next commitfest.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
I updated the patch as David Rowley mentioned.
Looks fine to me. Please add to the next commitfest.
Thanks. Added.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
---
Thanks and best regards,
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/
Huong Dangminh <huo-dangminh@ys.jp.nec.com> writes:
2018-04-11 0:13 GMT-03:00 David Rowley <david.rowley@2ndquadrant.com>:
I can recreate this when building with MSVC 2012. I confirm that I see
the same as you. Microsoft are setting errno to EDOM in the above 3
cases, where in Linux the result is still NaN, just the errno is not
set.
I updated the patch as David Rowley mentioned.
Pushed. I'd mainly note that you need to update all the variant float8
expected-files, not just the primary one. (Sometimes this requires a
bit of guesswork, but here we're expecting all platforms to produce
the same result. The buildfarm should tell us if I got it wrong.)
regards, tom lane
David Rowley <david.rowley@2ndquadrant.com> writes:
You patch fixes the problem for me, and importantly the two following
cases still work:
postgres=# select power(1,'NaN');
power
-------
1
(1 row)
postgres=# select power('NaN', 0);
power
-------
1
(1 row)
There's no mention in the SQL standard about NaN handling in the above
two cases, but I wonder if it's worth also adding a couple of tests
for the above two cases to ensure all platforms are doing the same
thing... ?
So the early returns from the buildfarm say "no, they aren't". It looks
like older BSDen know the NaN^0 = 1 rule, but they return NaN for 1^NaN.
Aside from the failure buildfarm member nightjar is showing, I've
confirmed this behavior on a NetBSD 5.2 installation I had laying around,
and there's also evidence in the related man page:
http://netbsd.gw.com/cgi-bin/man-cgi?pow+3+NetBSD-5.2.3
which goes to great lengths to justify NaN^0 = 1 while saying nothing
to suggest that 1^NaN might not yield NaN.
I'm not sure if we should add more special-case code for that, or just
remove that test case again. Historically we've not really felt that it
was our job to mask oddities of the platform's math library, so the fact
that power() is worrying about this seems like unjustified scope creep.
On the other hand, we do have a bunch of special cases there already,
so refusing to handle older BSD would be a tad inconsistent.
And, while we're on the subject ... some of my machines give
postgres=# select power(-1,'NaN');
ERROR: a negative number raised to a non-integer power yields a complex result
POSIX says this should return NaN, not an error (and my old NetBSD
installation does that, as does numeric_power). Should we change it?
I'm a bit inclined to handle all the NaN-input cases with explicit
code before we do anything that relies on libc's behavior.
BTW, numeric_power just emits NaN for NaN input, without either of
these special cases. But that's platform-independent, so I'm hesitant
to consider back-patching a change there; if we change that, I'd vote
for doing so only in HEAD.
regards, tom lane
On 30 April 2018 at 07:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Pushed. I'd mainly note that you need to update all the variant float8
expected-files, not just the primary one. (Sometimes this requires a
bit of guesswork, but here we're expecting all platforms to produce
the same result. The buildfarm should tell us if I got it wrong.)
gaur does not seem happy with this. I get the impression that pow(1,
NaN) and pow(NaN, 0) on that machine must be returning NaN and setting
errno to EDOM, and now that we're only using that code path when both
are are non-NaN we no longer hit the special case which does result =
1;
I wonder if it's better just to hard code these two cases before even
calling the pow() function.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
I wonder if it's better just to hard code these two cases before even
calling the pow() function.
Yeah, see my followup --- I also found out that SUSv2 (POSIX 1997)
doesn't require either of these special cases, which helps explain
why the inconsistency on older platforms.
Hacking on a patch now.
regards, tom lane
On 30 April 2018 at 09:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
http://netbsd.gw.com/cgi-bin/man-cgi?pow+3+NetBSD-5.2.3
which goes to great lengths to justify NaN^0 = 1 while saying nothing
to suggest that 1^NaN might not yield NaN.I'm not sure if we should add more special-case code for that, or just
remove that test case again. Historically we've not really felt that it
was our job to mask oddities of the platform's math library, so the fact
that power() is worrying about this seems like unjustified scope creep.
On the other hand, we do have a bunch of special cases there already,
so refusing to handle older BSD would be a tad inconsistent.
(Sorry missed your reply before I sent my last one)
Wouldn't this machine have returned 1 before this patch though? Surely
changing this behaviour this plaetform in favour of fixing on another
is worse than doing nothing. If that's the case, I think the only
option is to add a special case or revert this and document that the
behaviour may vary depending on the platform's implementation of
pow(). I think the special case is worth it, since there's already
some for the error cases.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
Wouldn't this machine have returned 1 before this patch though?
No, don't think so, because it doesn't set EDOM for the case.
Basically what we're doing here is making sure that we get results
conforming to current POSIX even on machines that predate that
standard. There are more of them floating around than I'd have
expected, but it still seems like a good change to make. Maybe
there's an argument for not back-patching, though?
regards, tom lane
On 30 April 2018 at 10:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <david.rowley@2ndquadrant.com> writes:
Wouldn't this machine have returned 1 before this patch though?
No, don't think so, because it doesn't set EDOM for the case.
Basically what we're doing here is making sure that we get results
conforming to current POSIX even on machines that predate that
standard. There are more of them floating around than I'd have
expected, but it still seems like a good change to make. Maybe
there's an argument for not back-patching, though?
I think we should back patch and try to be consistent about the
power(float8 1.0, 'NaN') and power('NaN', float8 0.0) cases. The
archives don't show any complaints about power() with NaN until this
one, so I imagine the number of people affected by this is small.
However, I think if we're willing to try to make MSVC consistent with
other platforms on this topic then there's no reason to draw the line
there and ignore other platforms that we claim to support.
POSIX seems like a good standard to follow for this in the absence of
guidance from the SQL standard.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
I updated the patch as David Rowley mentioned.
Pushed. I'd mainly note that you need to update all the variant float8
expected-files, not just the primary one. (Sometimes this requires a bit
Thank you.
I will be careful next time.
of guesswork, but here we're expecting all platforms to produce the same
result. The buildfarm should tell us if I got it wrong.)
Also thanks a lot of fixing failure in BSD platform as the result from buildfarm.
---
Thanks and best regards,
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/
On Sun, Apr 29, 2018 at 7:24 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
I think we should back patch and try to be consistent about the
power(float8 1.0, 'NaN') and power('NaN', float8 0.0) cases. The
archives don't show any complaints about power() with NaN until this
one, so I imagine the number of people affected by this is small.
I agree that this is not likely to affect a lot of people -- but the
question isn't how many people will be affected but rather, of those
that are, how many of them will be pleased rather than displeased by a
change. I would argue that the results have to be unambiguously wrong
in the back-branches to justify a change there, and this doesn't
appear to meet that standard. I would guess that the number of people
who use NaN is very small, but those people have probably adapted
their application to the behavior they are getting currently.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Apr 29, 2018 at 7:24 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:I think we should back patch and try to be consistent about the
power(float8 1.0, 'NaN') and power('NaN', float8 0.0) cases. The
archives don't show any complaints about power() with NaN until this
one, so I imagine the number of people affected by this is small.
I agree that this is not likely to affect a lot of people -- but the
question isn't how many people will be affected but rather, of those
that are, how many of them will be pleased rather than displeased by a
change. I would argue that the results have to be unambiguously wrong
in the back-branches to justify a change there, and this doesn't
appear to meet that standard. I would guess that the number of people
who use NaN is very small, but those people have probably adapted
their application to the behavior they are getting currently.
The point here, I think, is that you get behavior X on approximately 100%
of modern platforms, but (without this patch) behavior Y on some number of
older platforms. People who have tested their app on a modern platform
and then find that it misbehaves on an old one will think this is a bug
fix. People who only run their app on an old platform may think the
pre-patch behavior is fine, in which case they will indeed be upset if
we change it in a minor release. Are there more of the latter than the
former? I don't really know, and you don't either. But I don't think
we should discount the existence of the former category. Deploying
to production on an older release of $system than you develop on
is hardly an unusual scenario.
regards, tom lane
On Tue, May 1, 2018 at 1:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The point here, I think, is that you get behavior X on approximately 100%
of modern platforms, but (without this patch) behavior Y on some number of
older platforms. People who have tested their app on a modern platform
and then find that it misbehaves on an old one will think this is a bug
fix. People who only run their app on an old platform may think the
pre-patch behavior is fine, in which case they will indeed be upset if
we change it in a minor release. Are there more of the latter than the
former? I don't really know, and you don't either.
I agree with all of that.
But I don't think
we should discount the existence of the former category. Deploying
to production on an older release of $system than you develop on
is hardly an unusual scenario.
That's probably true, but making dev, test, and production boxes
similar is generally good practice and users can do as much or as
little of it as they find they need in order to avoid getting burned.
They can't do anything about behavior changes we inject into minor
releases.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, May 1, 2018 at 12:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:
But I don't think
we should discount the existence of the former category. Deploying
to production on an older release of $system than you develop on
is hardly an unusual scenario.That's probably true, but making dev, test, and production boxes
similar is generally good practice and users can do as much or as
little of it as they find they need in order to avoid getting burned.
They can't do anything about behavior changes we inject into minor
releases.
+1; this doesn't seem clear-cut and important enough to deviate from the
(for me) preferable position of leaving well-enough-alone in the back
branches.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Tue, May 1, 2018 at 12:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:
That's probably true, but making dev, test, and production boxes
similar is generally good practice and users can do as much or as
little of it as they find they need in order to avoid getting burned.
They can't do anything about behavior changes we inject into minor
releases.
+1; this doesn't seem clear-cut and important enough to deviate from the
(for me) preferable position of leaving well-enough-alone in the back
branches.
Well, I seem to be in the minority here, so unless additional people
chime in, I'll revert this in the back branches shortly.
regards, tom lane