Обсуждение: Re: pgsql: Add support for hyperbolic functions, as well as log10().

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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