Re: Bug in GiST paring heap comparator

On Wed, Sep 11, 2019 at 3:34 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> On 09.09.2019 22:47, Alexander Korotkov wrote:
>
> On Mon, Sep 9, 2019 at 8:32 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>
> On 08.09.2019 22:32, Alexander Korotkov wrote:
>
> On Fri, Sep 6, 2019 at 5:44 PM Alexander Korotkov
> <a(dot)korotkov(at)postgrespro(dot)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.

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

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company