Re: [HACKERS] [PATCH] Improve geometric types - Mailing list 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