On 07/31/2018 05:14 PM, Emre Hasegeli wrote:
>> 1) common_entry_cmp is still handling 'delta' as double, although the
>> CommonEntry was modified to use float8. IMHO it should also simply call
>> float8_cmp_internal instead of doing comparisons.
>
> I am changing it to define delta as "float8" and to use float8_cmp_internal().
>
>> 2) gist_box_picksplit does this
>>
>> int m = ceil(LIMIT_RATIO * (float8) nentries);
>>
>> instead of
>>
>> int m = ceil(LIMIT_RATIO * (double) nentries);
>>
>> which seems rather unnecessary, considering the only point of the cast was
>> probably to do more accurate multiplication. And it seems pointless to cast
>> it to float8 but then not use float8_mul().
>
> I am removing the cast.
>
>> 3) computeDistance does this:
>>
>> if (point->y > box->high.y)
>> result = float8_mi(point->y, box->high.y);
>> else if (point->y < box->low.y)
>> result = float8_mi(box->low.y, point->y);
>>
>> which seems suspicious. Shouldn't the comparisons be done by float8_lt and
>> float8_gt too? That's what we do elsewhere.
>
> I assumed the GiST code already handles NaNs correctly and tried not
> to change its behavior. It may be a good idea to revert existing NaN
> handling in favour of using the inline functions every time. Should I
> do that?
Ah, so there's an assumption that NaNs are handled earlier and never
reach this place? That's probably a safe assumption. I haven't thought
about that, it simply seemed suspicious that the code mixes direct
comparisons and float8_mi() calls.
>
>> 4) I think we should just get rid of GEODEBUG entirely. The preceding
>> patches removes about 20 out of 27 occurrences anyway, so let's ditch the
>> remaining few.
>
> I agree. Shall I append it to this patch?
>
Not sure, I'll leave that up to you. I don't mind doing it in a separate
patch (I'd probably prefer that over mixing it into unrelated patch).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services