On Wed, Dec 10, 2025 at 2:41 PM Victor Yegorov <[email protected]> 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
