Re: Avoid full GIN index scan when possible

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Thu, Aug 1, 2019 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Eyeing this a bit further ... doesn't scanPendingInsert also need
>> to honor so->forcedRecheck?  Something along the lines of

> I think so.

Yeah, it does --- the updated pg_trgm test attached fails if it doesn't.

Also, I found that Alexander's concern upthread:

>> What would happen when two-columns index have GIN_SEARCH_MODE_DEFAULT
>> scan on first column and GIN_SEARCH_MODE_ALL on second?  I think even
>> if triconsistent() for second column returns GIN_TRUE, we still need
>> to recheck to verify second columns is not NULL.

is entirely on-point.  This patch generates the wrong answer in the
case I added to gin.sql below.  (The expected output was generated
with HEAD and seems correct, but with these code changes, we incorrectly
report the row with NULL as matching.  So I expect the cfbot is going
to complain about the patch in this state.)

While I've not attempted to fix that here, I wonder whether we shouldn't
fix it by just forcing forcedRecheck to true in any case where we discard
an ALL qualifier.  That would get rid of all the ugliness around
ginFillScanKey, which I'd otherwise really want to refactor to avoid
this business of adding and then removing a scan key.  It would also
get rid of the bit about "XXX Need to use ALL mode instead of EVERYTHING
to skip NULLs if ALL mode has been seen", which aside from being ugly
seems to be dead wrong for multi-column-index cases.

BTW, it's not particularly the fault of this patch, but: what does it
even mean to specify GIN_SEARCH_MODE_ALL with a nonzero number of keys?
Should we decide to treat that as an error?  It doesn't look to me like
any of the in-tree opclasses will return such a case, and I'm not at
all convinced what the GIN scan code would actually do with it, except
that I doubt it matches the documentation.

Setting this back to Waiting on Author.

			regards, tom lane