On Wed, Dec 10, 2025 at 2:41 PM Victor Yegorov <vyegorov@gmail.com> wrote:
> Compiled and tested without issues.
Pushed. Thanks for the review!
> Small note: as you're removing “We rely on the convention that heap TIDs in the scanpos
> items array are stored in ascending heap TID order…” part of the comment, perhaps it'd be
> good to add smth similar to that to the “Sort and uniqueify so->killedItems[] to deal with all this.”
> piece? Smth like:
>
> + * Sort and uniqueify so->killedItems[] to deal with all this,
> + * as TIDs are processed in ascending order.
>
> I feel the need for such a comment from my POV as a code reader.
I did have a comment like that at one point, but I felt that it didn't
quite make sense to keep it. Such a comment would address how things
used to work, not how they work now (also how they really should have
worked all along).
With this change in place, we have a strict guarantee that the
contents of so->currPos.items[] will always be exactly the same when
scanning a page backwards as when scanning the same page forwards --
regardless of whether or not posting lists are involved, or any other
factor. In short, so->currPos.items[] will contain matching items in
index key space order in all cases. So ISTM that it doesn't make any
sense to draw attention to posting list TIDs within _bt_killitems. All
of that is strongly implied by the index key space order thing.
--
Peter Geoghegan
