When deciding whether to pop entries from the queue (line 191), the
comparison is done against scandesc->xs_orderbyvals, which as the comment
states are the last values physically returned by the index. I think this
is wrong, or at least pretty inefficient, in the case that the last index
tuple was lossy. What will frequently happen is that the comparison value
will be too small so we'll not be able to pop the queue item, whereas if
we'd compared against the corrected values in node->iss_OrderByValues,
we would have popped the item. I don't think this can result in a visible
failure, but for sure it can result in unnecessary queue traffic.
So ISTM we ought to preserve the state currently represented only in the
local variables lastfetched_vals/lastfetched_nulls, and use that in the
pop comparison.
OTOH, I've not yet consumed any caffeine today so maybe I missed
something.
One thing that is sadly lacking from this whole patch is any clear
specification of the behavior required from the index AM. Is it expected
to return items in the exact order implied by their reported (possibly
lossy) distance values? Or would it be okay, for instance, to return them
in the actually correct final order even though it returns lossy distance
values that are then not in order? (And if the latter is disallowed, why?)
Without such a specification it's impossible to construct an argument that
this queuing mechanism delivers correct answers.
regards, tom lane