Re: index prefetching

On Wed, Jul 8, 2026 at 6:14 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> 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.

That's right. I will push 0001 in the next 7-10 days, barring any objections.

> 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.

Cool.

Attached V30 addresses your feedback. It also uses the new shorter
"pg_always_inline" spelling to force function inlining.

> 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)?

I agree that that's a problem. It's not just the name, though; the
function itself uses private heap AM state, and therefore cannot
actually be used by third-party table AMs. V30 fixes this by renaming
the function, moving it into indexam.c, and refactoring it to use only
table-AM-agnostic state accessible from the caller's IndexScanDesc.

> 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.

These comments are a direct replacement for comments that were removed
from nodeIndexonlyscan.c (or close to it). Andres suggested that the
comments be reworded and moved above visibilitymap_get_status. There's
no obvious central place to move the comments to in
heapam_indexscan.c, so we're moving them to visibilitymap.c instead.

> 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.

I don't mind moving the comment into a separate, earlier commit, but
it doesn't seem important.

> 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.

I agree, that was/is really annoying.

> 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.

Here's what I said on June 7: it isn't strictly necessary for the
*selfuncs.c caller* to go through the same steps for "name" columns,
even though selfuncs.c uses the new slot-based interface. It'd just be
fragile to skip those steps: selfuncs.c could in the future do
something new that silently relies on those steps without anybody
noticing. Every caller to the new tableam.h interface gets that stuff
automatically going forward, so we don't have to worry about that
possibility anymore.

It *is* already strictly necessary to have special handling of "name"
columns during index-only scans for the main nodeIndexonlyscan.c case.
That's why David Rowley added the required extra handling to
StoreIndexTuple as part of bugfix commit a63224be49. The only change
in the patch series is that the relevant handling now takes place in a
utility function that we expect to be called by the table AM during
index-only scans (not from nodeIndexonlyscan.c, as on master).

As I mentioned already in passing, in V30 I've renamed
heap_fill_ios_slot to index_fill_ios_slot (also called StoreIndexTuple
on master), and moved it over to indexam.c. It is a truly reusable
utility function. Table AMs should be able to do everything required
by index-only scans using exactly the same approach as heapam, because
it just doesn't matter what format they store table tuples in/what
kind of slot they use for plain index scans -- index-only scans are
required to use a virtual slot in all cases, which we assert in
index_fill_ios_slot and in tableam.h, and so the same input and output
formats as heapam can be used.

Obviously, third party table AMs will still have to do IoS visibility
checks in their own way, but that has nothing to do with tuple/slot
formats. They'll be using xs_hitup, during GiST/SP-GiST IoSs. xs_hitup
happens to be a HeapTuple field, but that doesn't in itself doesn't
tie anything to heapam (it's just the convention followed by index AMs
that need to convert between on-disk and output formats in some custom
way).

> 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.

I agree that the comment as they appeared in V29 could be much clearer.

In V30 we have index_beginscan_internal comments that first explain
*why* we're doing this at all at a high level, pointing out that the
resources being allocated at that point are for use by the now-nearby
index_fill_ios_slot function. The "name" case is considered a special
case that is applied during all index-only scans that use a "name"
column for index AMs that produce IndexTuple format tuples (in
practice this is just nbtree, index AMs that want to store data in a
different type to the type being indexes have to use the xs_hitup
approach instead of the xs_itup approach).

> > * 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?

The case that comes to mind is one like "SELECT * FROM mytable ORDER
BY mycol LIMIT 1". Such a query could have many more index tuples
counted under this new scheme (possibly several hundred times more).
That isn't a tiny behavioral change. But more or less the same thing
already happens with filter quals (and with many kinds of bitmap
scans). You already can't treat it as "the number of tuples returned
by index [only] scan nodes" today.

I think that it makes sense for pgstat_count_index_tuples to just be a
mechanical account of how many index tuples were returned from the
index AM. That definition has the fewest problems, in my view.

What is the true purpose of pg_stat_all_indexes.idx_tup_read, anyway?
Personally I've never found idx_tup_read to be of much use. The basic
problem is that it is only a weak proxy for the actual cost of
returning index tuples from the index AM (the number of index pages
read is a much better proxy). Counting exactly the index tuples
returned from the index AM is at least consistent (across bitmap
scans, plain scans, and index-only scans) and easy to understand.

Note that this new pgstat_count_index_tuples scheme changes nothing at
all about how amgettuple index AMs increment idx_tup_read, so there's
no reason why disagreement on this idx_tup_read design question needs
to hold up the first patch.

--
Peter Geoghegan