On Thu, Aug 1, 2019 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > Attached v4 that should address all comments.
>
> Eyeing this a bit further ... doesn't scanPendingInsert also need
> to honor so->forcedRecheck? Something along the lines of
>
> - tbm_add_tuples(tbm, &pos.item, 1, recheck);
> + tbm_add_tuples(tbm, &pos.item, 1, recheck | so->forcedRecheck);
>
> at line 1837? (Obviously, there's more than one way you could
> write that.)
I think so.
> I'm also not exactly satisfied with the new comments --- they aren't
> conveying much, and the XXX in one of them is confusing; does that
> mean you're unsure that the comment is correct?
That's actually not my code, and I'm not familiar enough with GIN code
to do much better :(
For the XXX, IIUC Nikita added this comment as room for future
improvement, as stated in his initial mail:
>> 2. We need to replace GIN_SEARCH_MODE_EVERYTHING with GIN_SEARCH_MODE_ALL
>> if there are no GIN entries and some key requested GIN_SEARCH_MODE_ALL
>> because we need to skip NULLs in GIN_SEARCH_MODE_ALL. Simplest example here
>> is "array @> '{}'": triconsistent() returns GIN_TRUE, recheck is not forced,
>> and GIN_SEARCH_MODE_EVERYTHING returns NULLs that are not rechecked. Maybe
>> it would be better to introduce new GIN_SEARCH_MODE_EVERYTHING_NON_NULL.
> The added test case seems a bit unsatisfying as well, in that it
> fails to retrieve any rows. It's not very clear what it's
> trying to test.
Yes, I used the same tests as before, but since with this approach
there's no way to distinguish whether a full index scan was performed,
so the explain is quite useless. However, testing both cases should
still have the value to test the newly added code path.