[PATCH] Improve geometric types

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Hi Emre,

I'm afraid these patches conflict with current master branch.

The new status of this patch is: Waiting on Author

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I'm afraid these patches conflict with current master branch.

Rebased.

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

PostgreSQL fails with SIGSEGV during `make check-world`.

Backtrace: http://afiskon.ru/s/d4/f3dc17838a_sigsegv.txt
regression.diffs (not very useful): http://afiskon.ru/s/ac/ac5294656c_regression.diffs.txt
regression.out: http://afiskon.ru/s/70/39d872e2b8_regression.out.txt
How to reproduce: https://github.com/afiskon/pgscripts/blob/master/full-build.sh

The environment is Arch Linux x64, gcc 7.1.1

The new status of this patch is: Waiting on Author

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

PostgreSQL fails with SIGSEGV during `make check-world`.

Fixed. I am sorry for not running "check-world" before.

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

LGTM.

The new status of this patch is: Ready for Committer

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello, sorry to late for the party, but may I comment on this?

At Tue, 05 Sep 2017 13:18:12 +0000, Aleksander Alekseev <a.alekseev@postgrespro.ru> wrote in <20170905131812.18925.13551.pgcf@coridan.postgresql.org>

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

LGTM.

The new status of this patch is: Ready for Committer

The first patch reconstructs the operators in layers. These
functions are called very frequently when used. Some function are
already inlined in float.h but some static functions in float.h
also can be and are better be inlined. Some of *_internal,
point_construct, line_calculate_point and so on are the
candidates.

You removed some DirectFunctionCall to the functions within the
same file but other functions remain in the style,
ex. poly_center or on_sl. The function called from the former
seems large enough but the latter function calls a so small
function that it could be inlined. Would you like to make some
additional functions use C call (instead of DirectFunctionCall)
and inlining them?

This is not a fault of this patch, but some functions like on_pb
seems missing comment to describe what it is. Would you like to
add some?

In the second patch, the additional include fmgrprotos.h in
btree_gin.c seems needless. Some float[48] features were macros
so that they share the same expressions between float4 and
float8. They still seems sharing perfectly the same expressions
in float.h. Is there any reason for converting them into typed
inline functions?

In float.h, MAXDOUBLEWIDTH is redueced from 500 to 128, but the
exponent of double is up to 308 so it doesn't seem sufficient. On
the other hand we won't use non-scientific notation for extremely
large numbers and it requires (perhaps) up to 26 bytes in the
case. In the soruce code, most of them uses "%e" and one of them
uses '%g". %e always takes the format of
"-1.(17digits)e+308".. So it would be less than 26
characters.

=# set extra_float_digits to 3;
=# select -1.221423424320453e308::float8;
?column?
---------------------------
-1.22142342432045302e+308

man printf: (linux)

Style e is used if the exponent from its conversion is less than
-4 or greater than or equal to the precision.

So we should be safe to have a buffer with 26 byte length and 500
bytes will apparently too large and even 128 will be too loose in
most cases. So how about something like the following?

#define MINDOUBLEWIDTH 32
...
float4out@float.c<modified>:

int ndig = FLT_DIG + extra_float_digits;

if (ndig < 1)
ndig = 1;

len = snprintf(ascii, MINDOUBLEWIDTH + 1, "%+.*g", ndig, num);
if (len > MINDOUBLEWIDTH + 1)
{
ascii = (char *) repalloc(ascii, len);
if (snprintf(ascii, len, "%+.*e", ndig, num) > len)
error(ERROR, "something wrong happens...");
}

I don't think the if part can be used so there would be no
performance degradation, I believe.

----
I'd like to pause here.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello, sorry to late for the party, but may I comment on this?

Thank you for picking this up again.

The first patch reconstructs the operators in layers. These
functions are called very frequently when used. Some function are
already inlined in float.h but some static functions in float.h
also can be and are better be inlined. Some of *_internal,
point_construct, line_calculate_point and so on are the
candidates.

They are static functions. I though compiler can decide to inline
them. Do you think adding "inline" to the function signatures are
necessary?

You removed some DirectFunctionCall to the functions within the
same file but other functions remain in the style,
ex. poly_center or on_sl. The function called from the former
seems large enough but the latter function calls a so small
function that it could be inlined. Would you like to make some
additional functions use C call (instead of DirectFunctionCall)
and inlining them?

I tried to minimise my changes to make reviewing easier. I can make
"_internal" functions for the remaining DirectFunctionCall()s, if you
find it necessary.

This is not a fault of this patch, but some functions like on_pb
seems missing comment to describe what it is. Would you like to
add some?

I will add some on the next version.

In the second patch, the additional include fmgrprotos.h in
btree_gin.c seems needless.

It must be something I missed on rebase. I will remove it.

Some float[48] features were macros
so that they share the same expressions between float4 and
float8. They still seems sharing perfectly the same expressions
in float.h. Is there any reason for converting them into typed
inline functions?

Kevin Grittner suggested using inline functions instead of macros.
They are easier to use compared to macros, and avoid double-evaluation
hazards.

In float.h, MAXDOUBLEWIDTH is redueced from 500 to 128, but the
exponent of double is up to 308 so it doesn't seem sufficient. On
the other hand we won't use non-scientific notation for extremely
large numbers and it requires (perhaps) up to 26 bytes in the
case. In the soruce code, most of them uses "%e" and one of them
uses '%g". %e always takes the format of
"-1.(17digits)e+308".. So it would be less than 26
characters.

=# set extra_float_digits to 3;
=# select -1.221423424320453e308::float8;
?column?
---------------------------
-1.22142342432045302e+308

man printf: (linux)

Style e is used if the exponent from its conversion is less than
-4 or greater than or equal to the precision.

So we should be safe to have a buffer with 26 byte length and 500
bytes will apparently too large and even 128 will be too loose in
most cases. So how about something like the following?

#define MINDOUBLEWIDTH 32

Should it be same for float4 and float8?

...
float4out@float.c<modified>:

int ndig = FLT_DIG + extra_float_digits;

if (ndig < 1)
ndig = 1;

len = snprintf(ascii, MINDOUBLEWIDTH + 1, "%+.*g", ndig, num);
if (len > MINDOUBLEWIDTH + 1)
{
ascii = (char *) repalloc(ascii, len);
if (snprintf(ascii, len, "%+.*e", ndig, num) > len)
error(ERROR, "something wrong happens...");
}

I don't think the if part can be used so there would be no
performance degradation, I believe.

Wouldn't this change the output of the float datatypes? That would be
a backwards incompatible change.

I'd like to pause here.

I will submit new versions after your are done with your review.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

At Tue, 12 Sep 2017 19:30:44 +0200, Emre Hasegeli <emre@hasegeli.com> wrote in <CAE2gYzxDRvNdhrWQa7ym423uvHLWJXkqx=BoJePYMdrKPR1Zhg@mail.gmail.com>

Hello, sorry to late for the party, but may I comment on this?

Thank you for picking this up again.

The first patch reconstructs the operators in layers. These
functions are called very frequently when used. Some function are
already inlined in float.h but some static functions in float.h
also can be and are better be inlined. Some of *_internal,
point_construct, line_calculate_point and so on are the
candidates.

They are static functions. I though compiler can decide to inline
them. Do you think adding "inline" to the function signatures are
necessary?

C++ surely make just static functions inlined but I'm not sure C
compiler does that. "static" is a scope modifier and "inline" is
not (what kind of modifier is it?). In regard to gcc,

https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Inline.html#Inline

By declaring a function inline, you can direct GCC to make
calls to that function faster. <snip> You can also direct GCC
to try to integrate all “simple enough” functions into their
callers with the option -finline-functions.

I didn't find that postgresql uses the options so I think we
shouldn't expect that they are inlined automatically.

You removed some DirectFunctionCall to the functions within the
same file but other functions remain in the style,
ex. poly_center or on_sl. The function called from the former
seems large enough but the latter function calls a so small
function that it could be inlined. Would you like to make some
additional functions use C call (instead of DirectFunctionCall)
and inlining them?

I tried to minimise my changes to make reviewing easier. I can make
"_internal" functions for the remaining DirectFunctionCall()s, if you
find it necessary.

Ok, it would be another patch even if we need that.

This is not a fault of this patch, but some functions like on_pb
seems missing comment to describe what it is. Would you like to
add some?

I will add some on the next version.

In the second patch, the additional include fmgrprotos.h in
btree_gin.c seems needless.

It must be something I missed on rebase. I will remove it.

Some float[48] features were macros
so that they share the same expressions between float4 and
float8. They still seems sharing perfectly the same expressions
in float.h. Is there any reason for converting them into typed
inline functions?

Kevin Grittner suggested using inline functions instead of macros.
They are easier to use compared to macros, and avoid double-evaluation
hazards.

I recall a bit about the double-evaluation hazards. I think the
functions needs a comment describing the reasons so that anyone
kind won't try to merge them into a macro again.

In float.h, MAXDOUBLEWIDTH is redueced from 500 to 128, but the
exponent of double is up to 308 so it doesn't seem sufficient. On
the other hand we won't use non-scientific notation for extremely
large numbers and it requires (perhaps) up to 26 bytes in the
case. In the soruce code, most of them uses "%e" and one of them
uses '%g". %e always takes the format of
"-1.(17digits)e+308".. So it would be less than 26
characters.

=# set extra_float_digits to 3;
=# select -1.221423424320453e308::float8;
?column?
---------------------------
-1.22142342432045302e+308

man printf: (linux)

Style e is used if the exponent from its conversion is less than
-4 or greater than or equal to the precision.

So we should be safe to have a buffer with 26 byte length and 500
bytes will apparently too large and even 128 will be too loose in
most cases. So how about something like the following?

#define MINDOUBLEWIDTH 32

Should it be same for float4 and float8?

Strictly they are different each other but float8 needs 26 bytes
(additional 1 byte is the sign for expnent, which I forgot.) and
float4 needs 15 bytes so it could be reduced to 32 and 16
respectively.

| select -1.11111111111111111111111111e-38::float4;
| -1.11111113e-38
| select -1.11111111111111111111111111e-4::float4;
| -0.000111111112
| select -1.11111111111111111111111111e-5::float4;
| -1.11111112e-05

On the other hand float4 is anyway converted into double in
float4out and I'm not sure that the extension doesn't adds
spurious digits. Therefore I think it would be reasonable that
"%g" formatter requires up to 27 bytes (26 + terminating zero) so
it should use MINDOUBLEWIDTH regardless of the precision of the
value given to the formatter.

Addition to that if we are deliberately using %g in float4out, we
can use flot8out_internal instead of most of the stuff of
float4out.

| Datum
| float4out(PG_FUNCTION_ARGS)
| {
| float8 num = PG_GETARG_FLOAT4(0);
|
| PG_RETURN_CSTRING(float8out_internal(num));
| }

...
float4out@float.c<modified>:

int ndig = FLT_DIG + extra_float_digits;

if (ndig < 1)
ndig = 1;

len = snprintf(ascii, MINDOUBLEWIDTH + 1, "%+.*g", ndig, num);
if (len > MINDOUBLEWIDTH + 1)
{
ascii = (char *) repalloc(ascii, len);
if (snprintf(ascii, len, "%+.*e", ndig, num) > len)
error(ERROR, "something wrong happens...");
}

I don't think the if part can be used so there would be no
performance degradation, I believe.

Wouldn't this change the output of the float datatypes? That would be
a backwards incompatible change.

It just reduces the unused part after terminator \0, and we won't
get incompatible result.

I'd like to pause here.

I will submit new versions after your are done with your review.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Sep 14, 2017 at 3:33 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

I recall a bit about the double-evaluation hazards. I think the
functions needs a comment describing the reasons so that anyone
kind won't try to merge them into a macro again.

I think we can count on PostgreSQL developers to understand the
advantages of an inline function over a macro. Even if they don't,
the solution can't be to put a comment in every place where an inline
function is used explaining it. That would be very repetitive.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

At Thu, 14 Sep 2017 16:19:13 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobinBA7uvQifYaYGdDUoF6VTo56dvoTT6nKSpJF-Zfv5A@mail.gmail.com>

On Thu, Sep 14, 2017 at 3:33 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

I recall a bit about the double-evaluation hazards. I think the
functions needs a comment describing the reasons so that anyone
kind won't try to merge them into a macro again.

I think we can count on PostgreSQL developers to understand the
advantages of an inline function over a macro. Even if they don't,
the solution can't be to put a comment in every place where an inline
function is used explaining it. That would be very repetitive.

Of course putting such a comment to all inline functions is
silly. The point here is that many pairs of two functions with
exactly the same shape but handle different types are defined
side by side. Such situation seems tempting to merge them into
single macros, as the previous author did there.

So a simple one like the following would be enough.

/* don't merge the following same functions with different types
into single macros so that double evaluation won't happen */

Is it still too verbose?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

At Fri, 15 Sep 2017 17:23:28 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20170915.172328.97446299.horiguchi.kyotaro@lab.ntt.co.jp>

At Thu, 14 Sep 2017 16:19:13 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobinBA7uvQifYaYGdDUoF6VTo56dvoTT6nKSpJF-Zfv5A@mail.gmail.com>

On Thu, Sep 14, 2017 at 3:33 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

I recall a bit about the double-evaluation hazards. I think the
functions needs a comment describing the reasons so that anyone
kind won't try to merge them into a macro again.

I think we can count on PostgreSQL developers to understand the
advantages of an inline function over a macro. Even if they don't,
the solution can't be to put a comment in every place where an inline
function is used explaining it. That would be very repetitive.

Of course putting such a comment to all inline functions is
silly. The point here is that many pairs of two functions with
exactly the same shape but handle different types are defined
side by side. Such situation seems tempting to merge them into
single macros, as the previous author did there.

So a simple one like the following would be enough.

/* don't merge the following same functions with different types
into single macros so that double evaluation won't happen */

Is it still too verbose?

That being said, I'm not stick on that if Robert or others think
it as needless.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello, just one point on 0001.

The patch replace pg_hypot with hypot in libc. The man page says
as follows.

man 3 hypot

If the result overflows, a range error occurs, and the functions return
HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively.

..

ERRORS
See math_error(7) for information on how to determine whether an error
has occurred when calling these functions.

The following errors can occur:

Range error: result overflow
errno is set to ERANGE. An overflow floating-point exception
(FE_OVERFLOW) is raised.

Range error: result underflow
An underflow floating-point exception (FE_UNDERFLOW) is raised.

These functions do not set errno for this case.

So, the code seems to need some amendments following to this
spec.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Sep 15, 2017 at 4:23 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

/* don't merge the following same functions with different types
into single macros so that double evaluation won't happen */

Is it still too verbose?

Personally, I don't think such a comment has much value, but I am not
going to spend a lot of time arguing about it. It's really up to the
eventual committer to decide, and that probably won't be me in this
case. My knowledge of the geometric types isn't great, and I am a tad
busy breaking^Wimproving things I do understand.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

At Fri, 15 Sep 2017 11:25:30 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoYgg8=m9+Y54Az1r+KBpMuiEOZM_DLDF04jMP4twGR3Ng@mail.gmail.com>

On Fri, Sep 15, 2017 at 4:23 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

/* don't merge the following same functions with different types
into single macros so that double evaluation won't happen */

Is it still too verbose?

Personally, I don't think such a comment has much value, but I am not
going to spend a lot of time arguing about it. It's really up to the
eventual committer to decide, and that probably won't be me in this
case. My knowledge of the geometric types isn't great, and I am a tad
busy breaking^Wimproving things I do understand.

Ok, I agree with you.

# Though it is not a issue of geometric types :p

I'll withdrow the comment. Maybe someone notices of that when
reviewing such a patch.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

The new versions of the patches are attached addressing your comments.

C++ surely make just static functions inlined but I'm not sure C
compiler does that.

Thank you for your explanation. I marked the mentioned functions "inline".

So we should be safe to have a buffer with 26 byte length and 500
bytes will apparently too large and even 128 will be too loose in
most cases. So how about something like the following?

#define MINDOUBLEWIDTH 32

I left this part out for now. We can improve it separately.

The patch replace pg_hypot with hypot in libc. The man page says
as follows.

man 3 hypot

If the result overflows, a range error occurs, and the functions return
HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively.

..

ERRORS
See math_error(7) for information on how to determine whether an error
has occurred when calling these functions.

The following errors can occur:

Range error: result overflow
errno is set to ERANGE. An overflow floating-point exception
(FE_OVERFLOW) is raised.

Range error: result underflow
An underflow floating-point exception (FE_UNDERFLOW) is raised.

These functions do not set errno for this case.

So, the code seems to need some amendments following to this
spec.

I included them on the latest version.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello. Thank you for the new version.

0001: applies cleanly. regress passed.
this mainly refactoring geo_ops.c and replacing pg_hypot with hypot(3).
0002: applies cleanly. regress passed.
this just replaces float-ops macros into inline functions.
0003: applies cleanly. regress passed.
replaces double with float8 and bare arithmetic with defined functions.
0004: applies cleanly. regress passsed.
fix broken line-related functions.
I have some comments on this (later).

At Wed, 27 Sep 2017 17:44:52 +0200, Emre Hasegeli <emre@hasegeli.com> wrote in <CAE2gYzz2=335XEMHK-neNo=HgGskkPHQxUXkh8yDNsZAnCB5Bg@mail.gmail.com>

The new versions of the patches are attached addressing your comments.

C++ surely make just static functions inlined but I'm not sure C
compiler does that.

Thank you for your explanation. I marked the mentioned functions "inline".

Thanks.

So we should be safe to have a buffer with 26 byte length and 500
bytes will apparently too large and even 128 will be too loose in
most cases. So how about something like the following?

#define MINDOUBLEWIDTH 32

I left this part out for now. We can improve it separately.

Agreed. I found that psprintf does that with initial length of
128.

By the way, I found that MAXDOUBLEWIDTH has two inconsistent
definitions in formatting.c(500) and float.c(128). The definition
in new float.h is according to float.c and it seems better to be
untouched and it would be another issue.

At Wed, 27 Sep 2017 17:45:04 +0200, Emre Hasegeli <emre@hasegeli.com> wrote in <CAE2gYzz1KuT57vrO-XZk3KSqdpehAupJPLYu7AshuUwRF7MiMg@mail.gmail.com>

The patch replace pg_hypot with hypot in libc. The man page says
as follows.

...

I included them on the latest version.

# The commit message of 0001 says that "using C11 hypot()" but it
# is sine C99. I suppose we shold follow C99 at the time so I
# suggest rewrite it in the next version if any.

It seems bettern than expected. I confirmed that
pg_hypot(DBL_MAX, DBL_MAX) returned a value that is equivalent to
HUGE_VAL * HUGE_VAL on glibc, but I'm not sure the behavior on
other platforms is the same.

======
For other potential reviewers:

I found the origin of the function here.

/messages/by-id/4A90BD76.7070804@netspace.net.au
/messages/by-id/AANLkTim4cHELcGPf5w7Zd43_dQi_2RJ_b5_F_idSSbZI@mail.gmail.com

And the reason for pg_hypot is seen here.

/messages/by-id/407d949e0908222139t35ad3ad2q3e6b15646a27dd64@mail.gmail.com

I think the replacement is now acceptable according to the discussion.
======

And this should be the last comment of mine on the patch set.

In 0004, line_distance and line_interpt_internal seem
correct. Seemingly horizontal/vertical checks are redundant but
it is because unclearly defined EPSLON bahavior. lseg_perp seems
correct.

close_pl got a bug fix not only refactoring. I think it is
recommended to separate bugs and improvements, but I'm fine with
the current patch.

You added sanity check "A==0 && B==0" (in Ax + By + C) in
line_recv. I'm not sure the necessity of the check since it has
been checked in line_in but anyway the comparisons seem to be
typo(or thinko) of FPzero.

dist_pl is changed to take the smaller distance of both ends of
the segment. It seems absorbing error, so it might be better
taking the mean of the two distances. If you have a firm reason
for the change, it is better to be written there, or it might be
better left alone.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

And this should be the last comment of mine on the patch set.

Thank you. The new versions of the patches are attached addressing
your comments.

By the way, I found that MAXDOUBLEWIDTH has two inconsistent
definitions in formatting.c(500) and float.c(128). The definition
in new float.h is according to float.c and it seems better to be
untouched and it would be another issue.

The last version of the patch don't move these declarations to the header file.

# The commit message of 0001 says that "using C11 hypot()" but it
# is sine C99. I suppose we shold follow C99 at the time so I
# suggest rewrite it in the next version if any.

Changed.

close_pl got a bug fix not only refactoring. I think it is
recommended to separate bugs and improvements, but I'm fine with
the current patch.

I split the refactoring to the first patch.

You added sanity check "A==0 && B==0" (in Ax + By + C) in
line_recv. I'm not sure the necessity of the check since it has
been checked in line_in but anyway the comparisons seem to be
typo(or thinko) of FPzero.

Tom Lane suggested [1]/messages/by-id/11053.1466362319@sss.pgh.pa.us this one. I now made it use FPzero().

dist_pl is changed to take the smaller distance of both ends of
the segment. It seems absorbing error, so it might be better
taking the mean of the two distances. If you have a firm reason
for the change, it is better to be written there, or it might be
better left alone.

I don't really, so I left that part out.

[1]: /messages/by-id/11053.1466362319@sss.pgh.pa.us

On Mon, Oct 2, 2017 at 4:23 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

For other potential reviewers:

I found the origin of the function here.

/messages/by-id/4A90BD76.7070804@netspace.net.au
/messages/by-id/AANLkTim4cHELcGPf5w7Zd43_dQi_2RJ_b5_F_idSSbZI@mail.gmail.com

And the reason for pg_hypot is seen here.

/messages/by-id/407d949e0908222139t35ad3ad2q3e6b15646a27dd64@mail.gmail.com

I think the replacement is now acceptable according to the discussion.
======

I think if we're going to do this it should be separated out as its
own patch. Also, I think someone should explain what the reasoning
behind the change is. Do we, for example, foresee that the built-in
code might be faster or less likely to overflow? Because we're
clearly taking a risk -- most trivially, that the BF will break, or
more seriously, that some machines will have versions of this function
that don't actually behave quite the same.

That brings up a related point. How good is our test case coverage
for hypot(), especially in strange corner cases, like this one
mentioned in pg_hypot()'s comment:

* This implementation conforms to IEEE Std 1003.1 and GLIBC, in that the
* case of hypot(inf,nan) results in INF, and not NAN.

I'm potentially willing to commit a patch that just makes the
pg_hypot() -> hypot() change and does nothing else, if there are not
objections to that change, but I want to be sure that we'll know right
away if that turns out to break.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers