Re: 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/

2018-04-10 5:30 GMT-03:00 Huong Dangminh <huo-dangminh(at)ys(dot)jp(dot)nec(dot)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: underflow
>
> In 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(at)ys(dot)jp(dot)nec(dot)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: underflow
> >
> > In 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(at)ys(dot)jp(dot)nec(dot)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: 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 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], 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(dot)rowley(at)2ndquadrant(dot)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.

[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(dot)rowley(at)2ndquadrant(dot)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/

On 11 April 2018 at 16:42, Huong Dangminh <huo-dangminh(at)ys(dot)jp(dot)nec(dot)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(at)ys(dot)jp(dot)nec(dot)com> writes:
>> 2018-04-11 0:13 GMT-03:00 David Rowley <david(dot)rowley(at)2ndquadrant(dot)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(dot)rowley(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)rowley(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)rowley(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)rowley(at)2ndquadrant(dot)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(at)gmail(dot)com> writes:
> On Sun, Apr 29, 2018 at 7:24 PM, David Rowley
> <david(dot)rowley(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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(dot)g(dot)johnston(at)gmail(dot)com> writes:
> On Tue, May 1, 2018 at 12:08 PM, Robert Haas <robertmhaas(at)gmail(dot)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