On 10/03/2019 20:53, Peter Geoghegan wrote:
On Sun, Mar 10, 2019 at 7:09 AM Heikki Linnakangas <hlinn...@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
