Re: index prefetching

Hi,

On 7/8/26 01:00, Peter Geoghegan wrote:
> On Mon, Jun 22, 2026 at 4:48 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>> V28 is attached.
>
> V29 is attached. This revision focuses on avoiding small regressions
> in pgbench SELECT, particularly at higher client counts, where we're
> particularly sensitive to iTLB pressure. I'm posting this new revision
> now because V29 no longer applies cleanly; not much has changed since
> V28.
>

I took a quick look on 0001 today, because AFAIK you're intending
pushing it in not too distant future. I'll take a look at the other
parts too, but I wanted to post the 0001 feedback now.

In general, I think the slot-related improvements in 0001, explained in
the message from June 7th, make quite a bit of sense. I don't have a
particularly evolved taste when it comes to APIs, but I like most of the
changes implied by the 0001 changes.

A couple random comments for 0001 (not just about the slot stuff):

1) The comment at heap_fill_ios_slot says "This is exported for use by
other table AMs" but in that case isn't it a bit strange it's called
"heap_" (if it's meant for other table AMs)?

2) There's new comment before visibilitymap_get_status, explaining why
the info can't be too stale. Is that actually relevant to the changes in
this commit? I don't mind committing it with it, but it's not clear to
me if that's intentional.

3) I guess it'd be better to push the visibilitymap_get_status stuff
first, before 0001, right? I've already pinged the other thread.

4) Maybe it was in a previous version of the patches and I just did not
notice or forgot, but I really like how index_beginscan_internal does
all the work now. It was pretty confusing before, when it initialized
just some of the scan fields, and the caller then had to initialize the
rest. That has bitten me a couple times, I think.

5) I like how simple and clean nodeIndexonlyscan.c looks with the new
slot API. But I was really confused by the handling of "name" columns.
I'm not sure I quite understand why/if this is necessary, and in the
June 7 message you even say it's not a bug in practice. But assuming
it's needed, I think the comment in index_beginscan_internal() should
say more clearly why the block is needed, not just what it does. I know
it says "to size the workspace accordingly", but I'm not sure I'd
understand what that means from just the comment. Also, how come we
don't need something like this already?

> Highlights:
>
> * Microoptimization: index AMs are now allowed to set
> knownEndBackward/knownEndForward fields just as amgetbatch returns.
> This allows simple point lookup queries to call amgetbatch exactly
> once.
>
> * Microoptimization: pgstat_count_index_tuples now runs once per
> batch, not once per tuple as before. This slightly changes the
> semantics of the underlying counter, making it consistent with what
> we've always done with bitmap index scans: index tuples returned by
> the index AM are always counted, even when they're not returned to the
> core executor.
>

I think that seems OK. Are there cases where this would be confusing?

> * V29 also includes a new EXPLAIN (ANALYZE, IO) patch. This new patch
> shows standard read stream instrumentation info when EXPLAIN ANALYZE
> runs an index scan that performs IO prefetching.
>

Thanks. I know the patch needed some cleanup.

regards

--
Tomas Vondra