Re: [HACKERS] [PATCH] kNN for SP-GiST

Hi!

On Fri, Jun 29, 2018 at 5:37 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> On 06.03.2018 17:30, David Steele wrote:
>
> > I agree with Andres. Pushing this patch to the next CF.
>
> Attached 4th version of the patches rebased onto the current master.
> Nothing interesting has changed from the previous version.

I took a look to this patchset. In general, it looks good for me, but
I've following notes about it.

* We're going to replace scan stack with pairing heap not only for
KNN-search, but for regular search as well. Did you verify that it
doesn't cause regression for regular search case, because insertion
into pairing heap might be slower than insertion into stack? One may
say that it didn't caused regression in GiST, and that's why it
shouldn't cause regression in SP-GiST. However, SP-GiST trees might
be much higher and these performance aspects might be different.

* I think null handling requires better explanation. Nulls are
specially handled in pairingheap_SpGistSearchItem_cmp(). In the same
time you explicitly set infinity distances for leaf nulls. You
probably have reasons to implement it this way, but I think this
should be explained. Also isnull property of SpGistSearchItem doesn't
have comment.

* I think KNN support should be briefly described in
src/backed/access/spgist/README.

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