Re: index prefetching

On Mon, Oct 12, 2026 at 3:38 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> 1) I got a bit confused by tableam_index_pos_advance and how it
> interacts with tableam_index_pos_advance, and how it affects the "valid"
> flag of the position.

You can think of "valid" as representing the fact that it'd still be
safe to call tableam_index_pos_advance again with that same position,
at least in one scan direction (possibly just the opposite direction
to the one used by the most recent tableam_index_pos_advance call,
which can happen with a scrollable cursor).

BatchRingItemPos.item is the item most recently returned to the scan
-- not the next item that will be returned on the next call. If the
item most recently "returned" to the scan was "returned" only in the
sense that it made tableam_index_pos_advance return false/indicate
that there's no more matches in this direction, then it makes sense
for that item's batch-wise offset to be just-past-end at that time (or
just-before-start at that time, when scanning backwards).

tableam_index_pos_advance has assertions that verify that the caller
only ever passes "out of bounds" items that look exactly like these
expected cases. And so if, for example, a buggy table AM reached the
end of all matching items when scanning forward, and *then* asked for
*another* matching item in the forwards direction, we'll get an
assertion failure.

PortalRunSelect prevents this from ever happening -- it understands
when the scan has run out of tuples in a given direction, and knows
not to ask for more in that same direction. That makes it improbable
that even a buggy table AM could ever get this assertion failure.

> - if tableam_index_pos_advance succeeds, the position keeps valid=true
> (which seems logical) and points at a valid item in the batch
>
> - if tableam_index_pos_advance fails, the position still has valid=true
> (which may seem a bit surprising), and we do different things. If the
> ringbuffer is full (or we're throttling), we undo the position advance
> (thus making it point at a valid item again). But for BATCH_POS_DONE
> we don't do that, we leave it pointing kinda "out of bounds".

When we undo the tableam_index_pos_advance increment due to a full
ring buffer/throttling, we pause the read stream. We effectively
pretend that the most recent/pausing call to the read stream callback
never happened. This keeps the invariants consistent, including the
tableam_index_pos_advance assertions I just mentioned. That's a
special case.

> I'm not claiming this is broken, I just keep getting confused/distracted
> by this. I recall we discussed it in the past (a couple months back?).
> Maybe it'd be worth a comment somewhere?

Don't the comments in tableam_index_pos_advance suffice?

> 2) The explanation (both in docs and comments) when we start prefetching
> was talking about number of distinct blocks, but that's quite not what
> xs_blkswitch_count counts. [prefetch-start-docs.patch]

Seems reasonable, will do this.

> 3) It's probably worth mentioning that nearest-neighbor searches disable
> index-only scans in the relevant sgml docs. [gist-ios-ordering.patch]

Agreed.

> 4) I struggle to understand why read_stream_grow_repeats needs +1
> elements in stream->repeats. It seems pretty confusing, and also
> "infecting" various other places with the +1. Smells like off-by-one
> bugs in the future.

Fair. This was just a prototype.

> 5) I noticed show_scan_io_usage may not be sufficiently careful about
> checking iss_Instrument. I've been unable to make it fail with the
> in-core scans, but I suspect it might dereference NULL with custom
> scans or whatever. It probably should do the checks similarly to
> show_indexscan_info. That's my patch, so it's up to me to get that
> fixed (that patch probably needs more improvements).

Understood.

> I've been running a bunch of stress tests and benchmarks, looking for
> crashes and regressions, but that found nothing significant so far.

Cool.

--
Peter Geoghegan