Re: index prefetching

On Mon, Sep 14, 2026 at 10:36 AM Rui Zhao <zhaorui126(at)gmail(dot)com> wrote:
> I had a look at v34, focusing on 0001 since you're about to push it.
> All 12 patches apply cleanly to master (798bdcae89d).

Attached is v35, which addresses my outstanding concerns about include
dependencies. Some changes in v35 are similar to ones you
independently proposed. Even still, thanks for the review.

I will commit 0001 later this week. That kept getting held up by small
issues, but I finally think I've fixed all of them.

There's now src/backend/access/table/tableam_indexscan.c (and a
tableam_indexscan.h header), which is a selection of utilities that
table AMs use to implement the new slot-based index scan interface. In
practice most of its functions are only used during amgetbatch scans,
but that's just because amgetbatch requires more of them -- the file
itself is added by 0001/the slot commit. This gives 0001 a natural
place to put an inlinable tableam_index_getnext_tid (the
amgettuple-calling wrapper function), as well as
tableam_index_fill_ios_slot (the function that deals with filling an
IoS scan's slot using the fields set by index AMs such as
scan->xs_itup).

The amgetbatch commit adds src/backend/access/index/batchscan.c (and a
batchscan.h header), which contain utility functions for amgetbatch
index AMs. There's no real reason to place batchscan.c functions and
tableam_indexscan.c functions in the same file, which is how previous
versions did things.

There is now absolutely minimal coordination between batchscan.c and
tableam_indexscan.c. For example, tableam_indexscan.c.'s
release_and_unguard_batch function calls batchscan_release, even
though that's a function that is generally only supposed to be called
by index AMs. I don't think it's possible to *completely* avoid this
kind of thing. After all, the main goal of the amgetbatch design is to
have table AMs and index AMs cooperate more closely to allow maximum
freedom to reorder work. That goal fundamentally conflicts with having
*every* operation be the responsibility of exactly one layer (though
most operations still are).

3 totally generic accessors continue to live in relscan.h in v35.
relscan.h is where the IndexScanBatch struct is actually declared, and
where its layout is explained, so it seems natural to put these very
basic IndexScanBatch accessor functions there too.

As a consequence of this separation, v35 is much better at avoiding
the needless transitive inclusion of headers like pgstat.h. Splitting
things along table AM vs index AM lines had that effect -- pgstat.h is
only needed by the inline table AM helpers, and there's only one table
AM file that needs these helpers (namely heapam_indexscan.c). The only
file that has to include tableam_indexscan.h is heapam_indexscan.c
(barring guc_tables.c, which needs it for the index scan prefetching
GUC).

Other changes in v35:

* A new bugfix patch from Tomas fixes an issue where read_stream_reset
failed to reset pending_read_nblocks, leading to pathological
performance problems in certain rare cases.

This seems like a straightforward bug fix, so it should be okay to
commit without much delay. The problems that it fixes are rare, but
there's no reason to allow them.

* v35 fixes a bug that allowed SSI anomales during amgetbatch
index-only scans in previous versions.

We have to call PredicateLockPage immediately after the corresponding
VM_ALL_VISIBLE call -- it's not okay to delay it until the scan
actually consumes an item. In the amgetbatch path this could be long
after the same page's initial call to indicates that it is all
visible, allowing a concurrent writer to fail to observe our scan's
predicate lock.

I rewrote index-only-scan.spec to catch this bug, and other
PredicateLockPage bugs like it (since that isolation tester file was
created for that purpose). These changes appear in 0003.

* Renamed enable_indexscan_prefetch to
debug_disable_indexscan_prefetch (and inverted its meaning), since
it's really a developer option for debugging.

Users should tune prefetching in the usual way, through GUCs like
effective_io_concurrency. Fully disabling the use of a read stream has
been useful for testing, but I'm not even sure if it should remain in
the committed prefetching patch.

> 2. Since v34 is about index-only scan regressions, I also timed two
> index-only scans on pgbench_accounts_pkey (pgbench -s 5, everything in
> shared_buffers, VACUUMed so Heap Fetches is 0; -O2 without assertions;
> single client, -M prepared; 5 alternating 15-second rounds per build):
>
> point: SELECT aid FROM pgbench_accounts WHERE aid = :aid
> range: SELECT count(*) FROM pgbench_accounts
> WHERE aid BETWEEN :lo AND :lo + 9999
>
> median tps master master + 0001 full series
> point 42875 44583 (+4.0%) 43726 (+2.0%)
> range 1228 1228 (+0.0%) 1293 (+5.3%)
>
> (min-max over the 5 rounds: point 42076-43062 / 43930-44759 /
> 42302-44176; range 1209-1257 / 1212-1245 / 1290-1311)
>
> So with 0001 alone the 10k-row range scan is at parity and the point
> lookup is a bit faster; the full series is faster on both.

The changes in 0001 do seem to provide a nice win on ordinary range
scans that return several thousand matches. I saw about a 10% increase
in TPS with a variant pgbench workload like your one -- just with 0001
alone (most of the benefits are from improved code locality, which
mostly doesn't come from the amgetbatch changes).

In general, this project has required fixing many problems related to
code locality and code size. I finally think that I have those under
control. The remaining problems all relate to prefetching itself,
particularly issues with the read stream's heuristics that certain
index scans run into. I have a WIP patch that fixes some of these
problems by deduplicating nearby read stream block requests, but that
isn't quite ready to post yet.

> The header also declares index_fill_ios_slot(), the other helper meant
> only for table AMs, and its comment spells out what an xs_getnext_slot
> callback is expected to do, including maintaining
> scan->instrument->ntabletuplefetches, which so far is only stated in
> the commit message ("which table AMs are required to maintain"); a
> table AM that doesn't maintain it just shows Heap Fetches: 0 under
> EXPLAIN ANALYZE.

I don't think that case is worth drawing attention to. The same
applies to several pgstat functions that table AMs must call to
maintain accurate counters.

> On the "XXX Is there a more elegant way to do this?" next to heapam's
> hand-specialized copy of index_fill_ios_slot's IndexTuple case: I
> tried making index_fill_ios_slot() itself a static inline in the new
> header and dropping that copy. With the setup from point 2 the point
> lookup dropped from a median of 43885 tps (0001) to 40636 (all 5
> rounds below all 5 rounds of 0001), a second run of 6 alternating
> rounds gave -6% again, and the range scan was unchanged both times.

The original problem in this area that v34 set out to solve was
lowering added stack frames during very tight index-only scans.
Without inlining or specialization, the extra cross-TU call added
enough overhead to matter with such scans.

v35 simply inlines index_fill_ios_slot into heapam_indexscan.c's
index-only path. But the inline function calls a new
tableam_index_fill_ios_names helper function, which is a separate
pg_attribute_cold-marked function to deal with the extremely rare
special case of an nbtree index-only scan on an index with a "name"
index column stored by the index as cstring datums. This approach
seems to be more elegant and at least as efficient as the approach
taken in v34.

> 4. One behavioral difference with the full series applied (0001 alone
> matches master exactly here):

> The first index-only scan after the update reports Heap Fetches: 1325
> on master and 2000 with the series.

That's an inevitable consequence of eagerly setting VM status info in
a local cache stored within each batch. It's just an artefact of the
order in which the VM lookups happen relative to opportunistic VM
setting. I can't imagine that it could noticeably increase the number
of heap blocks read from disk, since of course the opportunistic
pruning/VM setting code path must have just read the same heap pages
with queries like your test query.

--
Peter Geoghegan