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
