Обсуждение: pgsql: Fix behavior of exp() and power() for infinity inputs.
Fix behavior of exp() and power() for infinity inputs. Previously, these functions tended to throw underflow errors for negative-infinity exponents. The correct thing per POSIX is to return 0, so let's do that instead. (Note that the SQL standard is silent on such issues, as it lacks the concepts of either Inf or NaN; so our practice is to follow POSIX whenever a corresponding C-library function exists.) Also, add a bunch of test cases verifying that exp() and power() actually do follow POSIX for Inf and NaN inputs. While this patch should guarantee that exp() passes the tests, power() will not unless the platform's pow(3) is fully POSIX-compliant. I already know that gaur fails some of the tests, and I am suspicious that the Windows animals will too; the extent of compliance of other old platforms remains to be seen. We might choose to drop failing test cases, or to work harder at overriding pow(3) for these cases, but first let's see just how good or bad the situation is. Discussion: https://postgr.es/m/582552.1591917752@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/decbe2bfb1051c5ab6c382b19e1d909e34227a22 Modified Files -------------- src/backend/utils/adt/float.c | 43 +++++++--- src/test/regress/expected/float8.out | 159 +++++++++++++++++++++++++++++++++++ src/test/regress/sql/float8.sql | 29 +++++++ 3 files changed, 221 insertions(+), 10 deletions(-)
On Sun, Jun 14, 2020 at 03:00:11PM +0000, Tom Lane wrote: > Fix behavior of exp() and power() for infinity inputs. > > Previously, these functions tended to throw underflow errors for > negative-infinity exponents. The correct thing per POSIX is to > return 0, so let's do that instead. (Note that the SQL standard > is silent on such issues, as it lacks the concepts of either Inf > or NaN; so our practice is to follow POSIX whenever a corresponding > C-library function exists.) > > Also, add a bunch of test cases verifying that exp() and power() > actually do follow POSIX for Inf and NaN inputs. While this patch > should guarantee that exp() passes the tests, power() will not unless > the platform's pow(3) is fully POSIX-compliant. I already know that > gaur fails some of the tests, and I am suspicious that the Windows > animals will too; the extent of compliance of other old platforms > remains to be seen. We might choose to drop failing test cases, or > to work harder at overriding pow(3) for these cases, but first let's > see just how good or bad the situation is. The Windows servers don't actually cay anything here, but sidewinder and hoverfly complain, on top of gaur of course: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2020-06-14%2015%3A05%3A33 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sidewinder&dt=2020-06-14%2018%3A45%3A28 -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Sun, Jun 14, 2020 at 03:00:11PM +0000, Tom Lane wrote: >> Also, add a bunch of test cases verifying that exp() and power() >> actually do follow POSIX for Inf and NaN inputs. While this patch >> should guarantee that exp() passes the tests, power() will not unless >> the platform's pow(3) is fully POSIX-compliant. I already know that >> gaur fails some of the tests, and I am suspicious that the Windows >> animals will too; the extent of compliance of other old platforms >> remains to be seen. We might choose to drop failing test cases, or >> to work harder at overriding pow(3) for these cases, but first let's >> see just how good or bad the situation is. > The Windows servers don't actually cay anything here, but sidewinder > and hoverfly complain, on top of gaur of course: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2020-06-14%2015%3A05%3A33 > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sidewinder&dt=2020-06-14%2018%3A45%3A28 Yeah, and presumably Noah's other AIX critters will fail too. I'm curious to see what damselfly will say; that's the other old-ish platform we have in the farm. Once that reports in, I'm intending to push the attached or something close to it. This is more or less the same thing for pow() as the current patch did for exp(): handle Inf cases manually and then simplify the error checks in the normal path. regards, tom lane diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c index 84d37de930..0fefef148c 100644 --- a/src/backend/utils/adt/float.c +++ b/src/backend/utils/adt/float.c @@ -1540,33 +1540,101 @@ dpow(PG_FUNCTION_ARGS) errmsg("a negative number raised to a non-integer power yields a complex result"))); /* - * pow() sets errno only on some platforms, depending on whether it - * follows _IEEE_, _POSIX_, _XOPEN_, or _SVID_, so we try to avoid using - * errno. However, some platform/CPU combinations return errno == EDOM - * and result == NaN for negative arg1 and very large arg2 (they must be - * 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. + * We don't trust the platform's pow() to handle infinity cases per spec + * either, so deal with those explicitly too. It's easier to handle + * infinite y first, so that it doesn't matter if x is also infinite. */ - errno = 0; - result = pow(arg1, arg2); - if (errno == EDOM && isnan(result)) + if (isinf(arg2)) { - if ((fabs(arg1) > 1 && arg2 >= 0) || (fabs(arg1) < 1 && arg2 < 0)) - /* The sign of Inf is not significant in this case. */ - result = get_float8_infinity(); - else if (fabs(arg1) != 1) - result = 0; - else - result = 1; + double absx = fabs(arg1); + + if (absx == 1.0) + result = 1.0; + else if (arg2 > 0.0) /* y = +Inf */ + { + if (absx > 1.0) + result = arg2; + else + result = 0.0; + } + else /* y = -Inf */ + { + if (absx > 1.0) + result = 0.0; + else + result = -arg2; + } } - else if (errno == ERANGE && result != 0 && !isinf(result)) - result = get_float8_infinity(); + else if (isinf(arg1)) + { + if (arg2 == 0.0) + result = 1.0; + else if (arg1 > 0.0) /* x = +Inf */ + { + if (arg2 > 0.0) + result = arg1; + else + result = 0.0; + } + else /* x = -Inf */ + { + bool yisoddinteger = false; - if (unlikely(isinf(result)) && !isinf(arg1) && !isinf(arg2)) - float_overflow_error(); - if (unlikely(result == 0.0) && arg1 != 0.0 && !isinf(arg1) && !isinf(arg2)) - float_underflow_error(); + if (arg2 == floor(arg2)) + { + /* integral; it's odd if y/2 is not integral */ + double halfy = arg2 * 0.5; /* should be computed exactly */ + + if (halfy != floor(halfy)) + yisoddinteger = true; + } + if (arg2 > 0.0) + result = yisoddinteger ? arg1 : -arg1; + else + result = yisoddinteger ? -0.0 : 0.0; + } + } + else + { + /* + * pow() sets errno on only some platforms, depending on whether it + * follows _IEEE_, _POSIX_, _XOPEN_, or _SVID_, so we must check both + * errno and invalid output values. (We can't rely on just the + * latter, either; some old platforms return a large-but-finite + * HUGE_VAL when reporting overflow.) + */ + errno = 0; + result = pow(arg1, arg2); + if (errno == EDOM || isnan(result)) + { + /* + * We eliminated all the possible domain errors above, or should + * have; but if pow() has a more restrictive test for "is y an + * integer?" than we do, we could get here anyway. Historical + * evidence suggests that some libm's once implemented that test + * as "y == (long) y", which of course misbehaves beyond LONG_MAX. + * There's not a lot of choice except to accept the platform's + * conclusion that we have a domain error. + */ + ereport(ERROR, + (errcode(ERRCODE_INVALID_ARGUMENT_FOR_POWER_FUNCTION), + errmsg("a negative number raised to a non-integer power yields a complex result"))); + } + else if (errno == ERANGE) + { + if (result != 0.0) + float_overflow_error(); + else + float_underflow_error(); + } + else + { + if (unlikely(isinf(result))) + float_overflow_error(); + if (unlikely(result == 0.0) && arg1 != 0.0) + float_underflow_error(); + } + } PG_RETURN_FLOAT8(result); }
On Mon, Jun 15, 2020 at 12:10:52AM -0400, Tom Lane wrote: > Yeah, and presumably Noah's other AIX critters will fail too. I'm curious > to see what damselfly will say; that's the other old-ish platform we have > in the farm. Once that reports in, I'm intending to push the attached or > something close to it. This is more or less the same thing for pow() as > the current patch did for exp(): handle Inf cases manually and then > simplify the error checks in the normal path. Thanks. What you have here looks consistent with what POSIX says: https://pubs.opengroup.org/onlinepubs/9699919799/functions/pow.html -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Mon, Jun 15, 2020 at 12:10:52AM -0400, Tom Lane wrote: >> Yeah, and presumably Noah's other AIX critters will fail too. I'm curious >> to see what damselfly will say; that's the other old-ish platform we have >> in the farm. Once that reports in, I'm intending to push the attached or >> something close to it. This is more or less the same thing for pow() as >> the current patch did for exp(): handle Inf cases manually and then >> simplify the error checks in the normal path. For the record, damselfly did indeed fail: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=damselfly&dt=2020-06-15%2011%3A23%3A34 So while it's pretty debatable whether we should care about obsolete NetBSD or HPUX versions, we do have an issue on both AIX and Illumos, which seems like sufficient reason to add the manual infinity handling. > Thanks. What you have here looks consistent with what POSIX says: > https://pubs.opengroup.org/onlinepubs/9699919799/functions/pow.html Right, it's based directly on the POSIX rules. One thing worth noting is that I changed what the code does if pow() returns EDOM. That's probably a dead code path on any modern platform, given that we already checked for invalid input. But if it did occur I think we should raise the same "domain error" message we use up top. There are two objections to the way the code stands: 1. It's effectively computing the result as though y is infinity, which (after the inf additions I propose here) we know isn't so. I think it makes more sense to decide "y isn't an integer even though we thought it was" than "y is infinite even though we thought it wasn't". Integer-ness, for float values too large to have any fractional bits, is a squishy concept, but infinite-ness isn't. 2. If we did want to conclude y is infinite, we should be prepared to return inf or 0 on that basis. What actually happens, given the following error checks, is either an underflow error, an overflow error, or returning 1.0. I don't think any of those is a sensible way of reporting EDOM. When that stanza of code was written, we had *none* of the special case checks that are now done before invoking pow(), so it had to consider a wider range of possibilities than it now does. I'm a little dubious that it was OK even then, but it's definitely off the mark now. regards, tom lane