On 10/03/2019 20:53, Peter Geoghegan wrote:
> On Sun, Mar 10, 2019 at 7:09 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> If we don't flip the meaning of the flag, then maybe calling it
>> something like "searching_for_leaf_page" would make sense:
>>
>> /*
>> * When set, we're searching for the leaf page with the given high key,
>> * rather than leaf tuples matching the search keys.
>> *
>> * Normally, when !searching_for_pivot_tuple, if a page's high key
>
> I guess you meant to say "searching_for_pivot_tuple" both times (not
> "searching_for_leaf_page"). After all, we always search for a leaf
> page. :-)
Ah, yeah. Not sure. I wrote it as "searching_for_pivot_tuple" first, but
changed to "searching_for_leaf_page" at the last minute. My thinking was
that in the page-deletion case, you're trying to re-locate a particular
leaf page. Otherwise, you're searching for matching tuples, not a
particular page. Although during insertion, I guess you are also
searching for a particular page, the page to insert to.
>> As the patch stands, you're also setting minusinfkey when dealing with
>> v3 indexes. I think it would be better to only set
>> searching_for_leaf_page in nbtpage.c.
>
> That would mean I would have to check both heapkeyspace and
> minusinfkey within _bt_compare().
Yeah.
> I would rather just keep the
> assertion that makes sure that !heapkeyspace callers are also
> minusinfkey callers, and the comments that explain why that is. It
> might even matter to performance -- having an extra condition in
> _bt_compare() is something we should avoid.
It's a hot codepath, but I doubt it's *that* hot that it matters,
performance-wise...
>> In general, I think BTScanInsert
>> should describe the search key, regardless of whether it's a V3 or V4
>> index. Properties of the index belong elsewhere. (We're violating that
>> by storing the 'heapkeyspace' flag in BTScanInsert. That wart is
>> probably OK, it is pretty convenient to have it there. But in principle...)
>
> The idea with pg_upgrade'd v3 indexes is, as I said a while back, that
> they too have a heap TID attribute. nbtsearch.c code is not allowed to
> rely on its value, though, and must use
> minusinfkey/searching_for_pivot_tuple semantics (relying on its value
> being minus infinity is still relying on its value being something).
Yeah. I find that's a complicated way to think about it. My mental model
is that v4 indexes store heap TIDs, and every tuple is unique thanks to
that. But on v3, we don't store heap TIDs, and duplicates are possible.
- Heikki