Re: [HACKERS] [PATCH] Improve geometric types

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