Horton

Thread

Commits

  1. Further adjust the tests for the hyperbolic functions.

  2. Adjust the tests for the hyperbolic functions.

  3. Rethink how to test the hyperbolic functions.

  4. Add support for hyperbolic functions, as well as log10().

  1. pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-12T19:55:14Z

    Add support for hyperbolic functions, as well as log10().

The SQL:2016 standard adds support for the hyperbolic functions
sinh(), cosh(), and tanh().  POSIX has long required libm to
provide those functions as well as their inverses asinh(),
acosh(), atanh().  Hence, let's just expose the libm functions
to the SQL level.  As with the trig functions, we only implement
versions for float8, not numeric.

For the moment, we'll assume that all platforms actually do have
these functions; if experience teaches otherwise, some autoconf
effort may be needed.

SQL:2016 also adds support for base-10 logarithm, but with the
function name log10(), whereas the name we've long used is log().
Add aliases named log10() for the float8 and numeric versions.

Lætitia Avrot

Discussion: https://postgr.es/m/CAB_COdguG22LO=rnxDQ2DW1uzv8aQoUzyDQNJjrR4k00XSgm5w@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/f1d85aa98ee71d9662309f6f0384b2f7f8f16f02

Modified Files
--------------
doc/src/sgml/func.sgml               | 107 ++++++++++++++++++++++-
src/backend/utils/adt/float.c        | 160 ++++++++++++++++++++++++++++++++++-
src/include/catalog/catversion.h     |   2 +-
src/include/catalog/pg_proc.dat      |  25 ++++++
src/test/regress/expected/float8.out |  37 ++++++++
src/test/regress/sql/float8.sql      |   8 ++
6 files changed, 333 insertions(+), 6 deletions(-)

  2. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Michael Paquier <michael@paquier.xyz> — 2019-03-13T02:21:59Z

    On Tue, Mar 12, 2019 at 07:55:14PM +0000, Tom Lane wrote:
> Add support for hyperbolic functions, as well as log10().
> 
> The SQL:2016 standard adds support for the hyperbolic functions
> sinh(), cosh(), and tanh().  POSIX has long required libm to
> provide those functions as well as their inverses asinh(),
> acosh(), atanh().  Hence, let's just expose the libm functions
> to the SQL level.  As with the trig functions, we only implement
> versions for float8, not numeric.

jacana is not a fan of this commit, and failed on float8:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-03-13%2000%3A00%3A27
@@ -476,7 +476,7 @@
SELECT asinh(float8 '0');
 asinh
-------
-     0
+    -0
(1 row)
--
Michael

  3. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-13T03:16:42Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Mar 12, 2019 at 07:55:14PM +0000, Tom Lane wrote:
>> Add support for hyperbolic functions, as well as log10().

> jacana is not a fan of this commit, and failed on float8:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-03-13%2000%3A00%3A27
> @@ -476,7 +476,7 @@
> SELECT asinh(float8 '0');
>  asinh
> -------
> -     0
> +    -0
> (1 row)

Yeah.  I warned Laetitia about not testing corner cases, but
it hadn't occurred to me that zero might be a corner case :-(

I'm inclined to leave it as-is for a day or so and see if any
other failures turn up, before deciding what to do about it.

			regards, tom lane

  4. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Michael Paquier <michael@paquier.xyz> — 2019-03-13T05:03:31Z

    On Tue, Mar 12, 2019 at 11:16:42PM -0400, Tom Lane wrote:
> Yeah.  I warned Laetitia about not testing corner cases, but
> it hadn't occurred to me that zero might be a corner case :-(

I was honestly expecting more failures than that when I saw the patch
landing.  This stuff is tricky :)

> I'm inclined to leave it as-is for a day or so and see if any
> other failures turn up, before deciding what to do about it.

Fine by me.
--
Michael

  5. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-13T21:56:32Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Mar 12, 2019 at 11:16:42PM -0400, Tom Lane wrote:
>> I'm inclined to leave it as-is for a day or so and see if any
>> other failures turn up, before deciding what to do about it.

> Fine by me.

Well, so far jacana is the only critter that's shown any problem.

I don't find any of the possible solutions to be super attractive:

1. Put in an explicit special case, along the lines of

	if (arg1 == 0.0)
		result = arg1;    /* Handle 0 and -0 explicitly */
	else
		result = asinh(arg1);

Aside from being ugly, this'd mean that our regression tests weren't
really exercising the library asinh function at all.

2. Drop that test case entirely, again leaving us with no coverage
of the asinh function.

3. Switch to some other test value besides 0.  This is also kinda ugly
because we almost certainly won't get identical results everywhere.
However, we could probably make the results pretty portable by using
extra_float_digits to suppress the low-order digit or two.  (If we did
that, I'd be inclined to do similarly for the other hyperbolic functions,
just so we're testing cases that actually show different results, and
thereby proving we didn't fat-finger which function we're calling.)

4. Carry an additional expected-results file.

5. Write our own asinh implementation.  Dean already did the work, of
course, but I think this'd be way overkill just because one platform
did their roundoff handling sloppily.  We're not in the business
of implementing transcendental functions better than libm does it.


Of these, probably the least bad is #3, even though it might require
a few rounds of experimentation to find the best extra_float_digits
setting to use.  I'll go try it without any roundoff, just to see
what the raw results look like ...

			regards, tom lane

  6. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-03-14T00:35:30Z

    On Wed, 13 Mar 2019, 21:56 Tom Lane, <tgl@sss.pgh.pa.us> wrote:

>
> Of these, probably the least bad is #3, even though it might require
> a few rounds of experimentation to find the best extra_float_digits
> setting to use.  I'll go try it without any roundoff, just to see
> what the raw results look like ...
>


Yeah, that seems like a reasonable thing to try.

I'm amazed that jacana's asinh() returned -0 for an input of +0. I'm not
aware of any implementation that does that. I'd be quite interested to know
what it returned for an input like 1e-20. If that returned any variety of
zero, I'd say that it's worse than useless. Another interesting test case
would be whether or not it satisfies asinh(-x) = -asinh(x) for a variety of
different values of x, because that's something that commonly breaks down
badly with naive implementations.

It's not unreasonable to expect these functions to be accurate to within
the last 1 or 2 digits, so testing with extra_float_digits or whatever
seems reasonable, but I think a wider variety of test inputs is required.

I also wonder if we should be doing what we do for the regular trig
functions and explicitly handle special cases like Inf and NaN to ensure
POSIX compatibility on all platforms.

Regards,
Dean


>

  7. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-14T00:48:49Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> It's not unreasonable to expect these functions to be accurate to within
> the last 1 or 2 digits, so testing with extra_float_digits or whatever
> seems reasonable, but I think a wider variety of test inputs is required.

Meh.  As I said before, we're not in the business of improving on what
libm does --- if someone has a beef with the results, they need to take
it to their platform's libm maintainer, not us.  The point of testing
this at all is just to ensure that we've wired up the SQL functions
to the library functions correctly.

> I also wonder if we should be doing what we do for the regular trig
> functions and explicitly handle special cases like Inf and NaN to ensure
> POSIX compatibility on all platforms.

I'm not too excited about this, but perhaps it would be interesting to
throw in tests of the inf/nan cases temporarily, just to see how big
a problem there is of that sort.  If the answer comes out to be
"all modern platforms get this right", I don't think it's our job to
clean up after the stragglers.  But if the answer is not that, maybe
I could be talked into spending code on it.

			regards, tom lane

  8. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Robert Haas <robertmhaas@gmail.com> — 2019-03-14T02:22:44Z

    On Wed, Mar 13, 2019 at 8:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Meh.  As I said before, we're not in the business of improving on what
> libm does --- if someone has a beef with the results, they need to take
> it to their platform's libm maintainer, not us.  The point of testing
> this at all is just to ensure that we've wired up the SQL functions
> to the library functions correctly.

Pretty sure we don't even need a test for that.  asinh() isn't going
to call creat() by mistake.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-14T02:39:03Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 13, 2019 at 8:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Meh.  As I said before, we're not in the business of improving on what
>> libm does --- if someone has a beef with the results, they need to take
>> it to their platform's libm maintainer, not us.  The point of testing
>> this at all is just to ensure that we've wired up the SQL functions
>> to the library functions correctly.

> Pretty sure we don't even need a test for that.  asinh() isn't going
> to call creat() by mistake.

No, but that's not the hazard.  I have a very fresh-in-mind example:
at one point while tweaking Laetitia's patch, I'd accidentally changed
datanh so that it called tanh not atanh.  The previous set of tests did
not reveal that :-(

			regards, tom lane

  10. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Robert Haas <robertmhaas@gmail.com> — 2019-03-14T02:40:18Z

    On Wed, Mar 13, 2019 at 10:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Mar 13, 2019 at 8:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Meh.  As I said before, we're not in the business of improving on what
> >> libm does --- if someone has a beef with the results, they need to take
> >> it to their platform's libm maintainer, not us.  The point of testing
> >> this at all is just to ensure that we've wired up the SQL functions
> >> to the library functions correctly.
>
> > Pretty sure we don't even need a test for that.  asinh() isn't going
> > to call creat() by mistake.
>
> No, but that's not the hazard.  I have a very fresh-in-mind example:
> at one point while tweaking Laetitia's patch, I'd accidentally changed
> datanh so that it called tanh not atanh.  The previous set of tests did
> not reveal that :-(

Well, that was a goof, but it's not likely that such a regression will
ever be reintroduced.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  11. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-14T02:44:30Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 13, 2019 at 10:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> No, but that's not the hazard.  I have a very fresh-in-mind example:
>> at one point while tweaking Laetitia's patch, I'd accidentally changed
>> datanh so that it called tanh not atanh.  The previous set of tests did
>> not reveal that :-(

> Well, that was a goof, but it's not likely that such a regression will
> ever be reintroduced.

Sure, but how about this for another example: maybe a given platform
hasn't got these functions (or they're in a different library we
didn't pull in), but you don't see a failure until you actually
call them.  We try to set up our link options so that that sort
of failure is reported at build time, but I wouldn't care to bet
that we've succeeded at that everywhere.

			regards, tom lane

  12. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-03-14T03:06:51Z

    On 3/13/19 5:56 PM, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Tue, Mar 12, 2019 at 11:16:42PM -0400, Tom Lane wrote:
>>> I'm inclined to leave it as-is for a day or so and see if any
>>> other failures turn up, before deciding what to do about it.
>> Fine by me.
> Well, so far jacana is the only critter that's shown any problem.
>
> I don't find any of the possible solutions to be super attractive:
>
> 1. Put in an explicit special case, along the lines of
>
> 	if (arg1 == 0.0)
> 		result = arg1;    /* Handle 0 and -0 explicitly */
> 	else
> 		result = asinh(arg1);
>
> Aside from being ugly, this'd mean that our regression tests weren't
> really exercising the library asinh function at all.


Or we could possibly call the function and then turn a result of -0 into 0?


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  13. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-14T03:18:27Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> Or we could possibly call the function and then turn a result of -0 into 0?

But -0 is the correct output if the input is -0.  So that approach
requires distinguishing -0 from 0, which is annoyingly difficult.

			regards, tom lane

  14. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-03-14T03:30:00Z

    At Wed, 13 Mar 2019 23:18:27 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <2503.1552533507@sss.pgh.pa.us>
tgl> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
tgl> > Or we could possibly call the function and then turn a result of -0 into 0?
tgl> 
tgl> But -0 is the correct output if the input is -0.  So that approach
tgl> requires distinguishing -0 from 0, which is annoyingly difficult.

I think just turning both of -0 and +0 into +0 works, and, FWIW,
it is what is done in geo_ops.c (e.g. line_construct()) as a kind
of normalization and I think it is legit for geo_ops, but I don't
think so for fundamental functions like (d)asinh().

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-14T04:41:27Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> I'm amazed that jacana's asinh() returned -0 for an input of +0.

Even more amusingly, it returns NaN for acosh('infinity'), cf
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-03-14%2003%3A00%3A34

Presumably that means they calculated "infinity - infinity" at some
point, but why?

So far, no other failures ...

			regards, tom lane

  16. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-03-14T08:28:16Z

    On Thu, 14 Mar 2019 at 04:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> > I'm amazed that jacana's asinh() returned -0 for an input of +0.
>
> Even more amusingly, it returns NaN for acosh('infinity'), cf
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-03-14%2003%3A00%3A34
>
> Presumably that means they calculated "infinity - infinity" at some
> point, but why?
>

Given the -0 result, I don't find that particularly surprising. I
suspect lots of formulae would end up doing that without proper
special-case handling upfront.

It looks like that's the only platform that isn't POSIX compliant
though, so maybe it's not worth worrying about.

Regards,
Dean

  17. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-03-14T12:54:49Z

    On 3/14/19 12:41 AM, Tom Lane wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> I'm amazed that jacana's asinh() returned -0 for an input of +0.
> Even more amusingly, it returns NaN for acosh('infinity'), cf
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-03-14%2003%3A00%3A34
>
> Presumably that means they calculated "infinity - infinity" at some
> point, but why?
>
> So far, no other failures ...
>
> 			



I have replicated this on my Msys2 test system.


I assume it's a bug in the mingw math library. I think jacana is the
only currently reporting mingw member :-( The MSVC members appear to be
happy.


I have several releases of the mingw64 toolsets installed on jacana -
I'll try an earlier version to see if it makes a difference.


cheers


andrew


-- 

Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  18. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-14T19:08:16Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 3/14/19 12:41 AM, Tom Lane wrote:
>> So far, no other failures ...

> I have replicated this on my Msys2 test system.
> I assume it's a bug in the mingw math library. I think jacana is the
> only currently reporting mingw member :-( The MSVC members appear to be
> happy.
> I have several releases of the mingw64 toolsets installed on jacana -
> I'll try an earlier version to see if it makes a difference.

Yeah, it would be interesting to know whether it's consistent across
different mingw versions.

So far, though, jacana is still the only buildfarm animal that's having
trouble with those tests as of c015f853b.  I want to wait another day or
so in hopes of getting more reports from stragglers.  But assuming that
that stays true, I do not feel any need to try to work around jacana's
issues.  We already have proof of two deficiencies in their
hyoerbolic-function code, and considering the tiny number of test cases
we've tried, it'd be folly to think there are only two.  I don't want
to embark on a project to clean that up for the sake of one substandard
implementation.

I feel therefore that what we should do (barring new evidence) is either

1. Remove all the inf/nan test cases for the hyoerbolic functions, on
the grounds that they're not really worth expending buildfarm cycles on
in the long run; or

2. Just comment out the one failing test, with a note about why.

I haven't got a strong preference as to which.  Thoughts?

			regards, tom lane

  19. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-03-14T21:09:30Z

    On 3/14/19 3:08 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 3/14/19 12:41 AM, Tom Lane wrote:
>>> So far, no other failures ...
>> I have replicated this on my Msys2 test system.
>> I assume it's a bug in the mingw math library. I think jacana is the
>> only currently reporting mingw member :-( The MSVC members appear to be
>> happy.
>> I have several releases of the mingw64 toolsets installed on jacana -
>> I'll try an earlier version to see if it makes a difference.
> Yeah, it would be interesting to know whether it's consistent across
> different mingw versions.



Tried with mingw64-gcc-5.4.0  (jacana is currently on 8.1.0). Same result.


>
> So far, though, jacana is still the only buildfarm animal that's having
> trouble with those tests as of c015f853b.  I want to wait another day or
> so in hopes of getting more reports from stragglers.  But assuming that
> that stays true, I do not feel any need to try to work around jacana's
> issues.  We already have proof of two deficiencies in their
> hyoerbolic-function code, and considering the tiny number of test cases
> we've tried, it'd be folly to think there are only two.  I don't want
> to embark on a project to clean that up for the sake of one substandard
> implementation.
>
> I feel therefore that what we should do (barring new evidence) is either
>
> 1. Remove all the inf/nan test cases for the hyoerbolic functions, on
> the grounds that they're not really worth expending buildfarm cycles on
> in the long run; or
>
> 2. Just comment out the one failing test, with a note about why.
>
> I haven't got a strong preference as to which.  Thoughts?
>
> 			


2. would help us memorialize the problem.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  20. Re: pgsql: Add support for hyperbolic functions, as well as log10().

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-16T19:51:37Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 3/14/19 3:08 PM, Tom Lane wrote:
>> I feel therefore that what we should do (barring new evidence) is either
>> 1. Remove all the inf/nan test cases for the hyoerbolic functions ...
>> 2. Just comment out the one failing test, with a note about why.

> 2. would help us memorialize the problem.

Hearing no other comments, done that way.

			regards, tom lane