Re: [PATCH] kNN for btree

On Mon, Sep 9, 2019 at 11:28 PM Alexander Korotkov
<a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> On Sun, Sep 8, 2019 at 11:35 PM Alexander Korotkov
> <a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> > I'm going to push 0001 changing "attno >= 1" to assert.
>
> Pushed. Rebased patchset is attached. I propose to limit
> consideration during this commitfest to this set of 7 remaining
> patches. The rest of patches could be considered later. I made some
> minor editing in preparation to commit. But I decided I've couple
> more notes to Nikita.
>
> * 0003 extracts part of fields from BTScanOpaqueData struct into new
> BTScanStateData struct. However, there is a big comment regarding
> BTScanOpaqueData just before definition of BTScanPosItem. It needs to
> be revised.
> * 0004 adds "knnState" field to BTScanOpaqueData in addition to
> "state" field. Wherein "knnState" might unused during knn scan if it
> could be done in one direction. This seems counter-intuitive. Could
> we rework this to have "forwardState" and "backwardState" fields
> instead of "state" and "knnState"?

I have reordered patchset into fewer more self-consistent patches.

Besides comments, code beautification and other improvements, now
there are dedicated fields for forward and backward scan states. The
forward scan state is the pointer to data structure, which is used in
ordinal unidirectional scan. So, it's mostly cosmetic change, but it
improves the code readability.

One thing bothers me. We have to move scalar distance operators from
btree_gist to core. btree_gist extension upgrade script have to
qualify operators with schema in order to distinguish core and
extension implementations. So, I have to use @extschema(at)(dot) But it's
forbidden for relocatable extensions. For now, I marken btree_gist as
non-relocatable. Our comment in extension.c says "For a relocatable
extension, we needn't do this. There cannot be any need for
@extschema@, else it wouldn't be relocatable.". Is it really true? I
think now we have pretty valid example for relocatable extension,
which needs @extschema@ in upgrade script. Any thoughts?

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