On 10/12/26 23:01, Peter Geoghegan wrote:
> On Mon, Oct 12, 2026 at 3:38 PM Tomas Vondra <[email protected]> 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?
>
Not sure. It probably does, but it's a bit unclear as it's explaining it
as a tableam_index_pos_advance precondition, i.e. what "valid" means at
the moment of calling that function.
The description you posted above (esp. value=true means it's safe to
call _advance) seems clearer to me. Maybe it's ultimately saying the
same thing, but it seems clearer to me, maybe we could use some fo the
in a comment somewhere.
>> 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.
>
Ack. I'll get to that after pgconf.eu, probably.
>> I've been running a bunch of stress tests and benchmarks, looking for
>> crashes and regressions, but that found nothing significant so far.
>
> Cool.
>
I'll keep running and tweaking the tests, but so far I found nothing
worth mentioning.
regards
--
Tomas Vondra
