Re: Bug in GiST paring heap comparator

On 12.09.2019 16:45, Alexander Korotkov wrote:
> On Wed, Sep 11, 2019 at 3:34 AM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>> On 09.09.2019 22:47, Alexander Korotkov wrote:
>>
>> On Mon, Sep 9, 2019 at 8:32 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>>
>> On 08.09.2019 22:32, Alexander Korotkov wrote:
>>
>> On Fri, Sep 6, 2019 at 5:44 PM Alexander Korotkov
>> <a.korotkov@postgrespro.ru> wrote:
>>
>> I'm going to push both if no objections.
>>
>> So, pushed!
>>
>> Two years ago there was a similar patch for this issue:
>> https://www.postgresql.org/message-id/1499c9d0-075a-3014-d2aa-ba59121b3728%40postgrespro.ru
>>
>> Sorry that I looked at this thread too late.
>>
>> I see, thanks.
>>
>> You patch seems a bit less cumbersome thanks to using GISTDistance
>> struct.  You don't have to introduce separate array with null flags.
>> But it's harder too keep index_store_float8_orderby_distances() used
>> by both GiST and SP-GiST.
>>
>> What do you think?  You can rebase your patch can propose that as refactoring.
>>
>> Rebased patch with refactoring is attached.
>>
>> During this rebase I found that SP-GiST code has similar problems with NULLs.
>> All SP-GiST functions do not check SK_ISNULL flag of ordering ScanKeys, and
>> that leads to segfaults.  In the following test, index is searched with
>> non-NULL key first and then with NULL key, that leads to a crash:
>>
>> CREATE TABLE t AS SELECT point(x, y) p
>> FROM generate_series(0, 100) x, generate_series(0, 100) y;
>>
>> CREATE INDEX ON t USING spgist (p);
>>
>> -- crash
>> SELECT (SELECT p FROM t ORDER BY p <-> pt LIMIT 1)
>> FROM (VALUES (point '1,2'), (NULL)) pts(pt);
>>
>>
>> After adding SK_ISNULL checks and starting to produce NULL distances, we need to
>> store NULL flags for distance somewhere.  Existing singleton flag for the whole
>> SPGistSearchItem is not sufficient anymore.
>>
>>
>> So, I introduced structure IndexOrderByDistance and used it everywhere in the
>> GiST and SP-GiST code instead of raw double distances.
>>
>>
>> SP-GiST order-by-distance code can be further refactored so that user functions
>> do not have to worry about SK_ISNULL checks.
> It doesn't seem SP-GiST inner_consistent and leaf_consistent functions
> can handle NULL scan keys for now.  So should we let them handle NULL
> orderby keys?  If we assume that NULL arguments produce NULL
> distances, we can evade changing the code of opclasses.
I have moved handling of NULL ordering keys from opclasses to the common
SP-GiST code, but really I don't like how it is implemented now. Maybe it's
worth to move handling of NULL order-by keys to the even more higher 
level so,
that AM don't have to worry about NULLs?

Also I leaved usages of IndexOrderByDistance in opclasses. I think, that 
may
help to minimize opclass changes in the future.

> Also, I've noticed IndexOrderByDistance is missed in typedefs.list.
Fixed.

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