Re: Review: B-Tree emulation for GIN

Teodor Sigaev <teodor(at)sigaev(dot)ru> writes:
> [ btree_gin 0.12 ]

Committed with some editorializations. There are still a few loose
ends:

* the question about zero-key queries that I mentioned before

* After this new look at the code I think that matchPartialInPendingList
is completely broken. Surely it's an infinite loop if the
comparePartial function returns -1. I also don't understand why it
stops short of examining the tuple at maxoff, or for that matter why
it's doing any searching at all when the code that's calling it seems
to be implementing a binary search. I think that part of the code
needs more testing and commenting.

* In keyGetItem(), it's not at all clear what the assumptions are for
the contents of the entryRes[] array and for the interaction with a
LossyPage entry. Is it intentional that you run through the array
comparing to a LossyPage key before deciding to exit the loop? It
seems like that might be affecting the behavior at the next call,
but if so you're making some undocumented assumptions about the way
comparison of a lossy-page pointer is going to behave. I thought it'd
be cleaner to move the ItemPointerIsLossyPage check up to before that
loop (right after the ItemPointerIsMax test) but the more I look at it
the less sure I am if that would break things. That code needs more
commenting too.

* I'd also like to come to some agreement about getting rid of the
fail-on-NULL-scankey problem in newScanKey(). As I noted in the
comment there, we could make that work cleanly if we are willing to
assume that all GIN-indexable operators are strict. We already assume
the same for hash and btree operators, so it doesn't seem like a big
problem to do this, but I wonder if there are any objections.

regards, tom lane