Re: Avoid full GIN index scan when possible

Hi!

Thank you for feedback!

On Sat, Jan 11, 2020 at 3:19 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> On Sat, Jan 11, 2020 at 1:10 AM Alexander Korotkov
> <a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> >
> > On Fri, Jan 10, 2020 at 7:36 PM Alexander Korotkov
> > <a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> > >
> > > On Fri, Jan 10, 2020 at 6:31 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >>
> > >> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > >> > So, I think v10 is a version of patch, which can be committed after
> > >> > some cleanup. And we can try doing better nulls handling in a separate
> > >> > patch.
> > >>
> > >> The cfbot reports that this doesn't pass regression testing.
> > >> I haven't looked into why not.
> > >
> > >
> > > Thank you for noticing. I'll take care of it.
> >
> >
> > In the v10 I've fixed a bug with nulls handling, but it appears that
> > test contained wrong expected result. I've modified this test so that
> > it directly compares sequential scan results with bitmap indexscan
> > results.
>
> Thanks a lot for working on that! I'm not very familiar with gin
> internals so additional eyes are definitely needed here but I agree
> that this approach is simpler and cleaner. I didn't find any problem
> with the modified logic, the patch applies cleanly, compiles without
> warning and all regression tests pass, so it all seems good to me.
>
> Here are a few comments:
>
> - In keyGetItem(), it seems that some comments would need to be
> updated wrt. the new excludeOnly flag. I'm thinking of:
>
> * Ok, we now know that there are no matches < minItem.
> *
> * If minItem is lossy, it means that there were no exact items on the
> * page among requiredEntries, because lossy pointers sort after exact
> * items. However, there might be exact items for the same page among
> * additionalEntries, so we mustn't advance past them.
>
> and
>
> /*
> * Normally, none of the items in additionalEntries can have a curItem
> * larger than minItem. But if minItem is a lossy page, then there
> * might be exact items on the same page among additionalEntries.
> */ if (ginCompareItemPointers(&entry->curItem, &minItem) < 0)
> {
> Assert(ItemPointerIsLossyPage(&minItem) || key->nrequired == 0);
> minItem = entry->curItem;
> }

Sure, thank you for pointing. I'm working on improving comments.
I'll provide updated patch soon.

> While at it, IIUC only excludeOnly key can have nrequired == 0 (if
> that's the case, this could be explicitly said in startScanKey
> relevant comment), so it'd be more consistent to also use excludeOnly
> rather than nrequired in this assert?

Make sense. I'll adjust the assert as well as comment.

> - the pg_trgm regression tests check for the number of rows returned
> with the new "excludeOnly" permutations, but only with an indexscan,
> should we make sure that the same results are returned with a seq
> scan?

Yes, I recently fixed similar issue in gin regression test. I'll
adjust pg_trgm test as well.

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