Re: compress method for spgist - 2

On 21.09.2017 02:27, Alexander Korotkov wrote:

> On Thu, Sep 21, 2017 at 2:06 AM, Darafei "Komяpa" Praliaskouski
> <me(at)komzpa(dot)net <mailto:me(at)komzpa(dot)net>> wrote:
>
> It is possible for bbox->low.x to be NaN when circle->center.x
> is and
> circle->radius are both +Infinity.
>
>
> What is rationale behind this circle?
>
>
> I would prefer to rather forbid any geometries with infs and nans. 
> However, then upgrade process will suffer.  User with such geometries
> would get errors during dump/restore, pg_upgraded instances would
> still contain invalid values...
>
> It seems to me that any circle with radius of any Infinity should
> become a [-Infinity .. Infinity, -Infinity .. Infinity] box.Then
> you won't have NaNs, and index structure shouldn't be broken.
>
>
> We probably should produce [-Infinity .. Infinity, -Infinity ..
> Infinity] box for any geometry containing inf or nan.  That MBR would
> be founded for any query, saying: "index can't help you for this kind
> value, only recheck can deal with that".  Therefore, we would at least
> guarantee that results of sequential scan and index scan are the same.
>

I have looked at the GiST KNN code and found the same errors for NaNs,
infinities and NULLs as in my SP-GiST KNN patch.

Attached two patches:

1. Fix NaN-inconsistencies in circle's bounding boxes computed in GiST
compress
method leading to the failed Assert(box->low.x <= box->high.x) in
computeDistance() from src/backend/access/gist/gistproc.c by
transforming NaNs
into infinities (corresponding test case provided in the second patch).

2. Fix ordering of NULL, NaN and infinite distances by GiST.  This distance
values could be mixed because NULL distances were transformed into
infinities,
and there was no special processing for NaNs in KNN queue's comparison
function.
At first I tried just to set recheck flag for NULL distances, but it did not
work for index-only scans because they do not support rechecking. So I had
to add a special flag for NULL distances.

Should I start a separate thread for this issue and add patches to
commitfest?

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company