Horton

Re: index prefetching

Peter Geoghegan <pg@bowt.ie>

View thread View on postgr.es
From: Peter Geoghegan <pg@bowt.ie>
To: Andres Freund <andres@anarazel.de>
Cc: Tomas Vondra <tomas@vondra.me>, Alexandre Felipe <o.alexandre.felipe@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Nazir Bilal Yavuz <byavuz81@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Melanie Plageman <melanieplageman@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Georgios <gkokolatos@protonmail.com>, Konstantin Knizhnik <knizhnik@garret.ru>, Dilip Kumar <dilipbalaut@gmail.com>
Date: 2026-04-06T05:47:43Z
Lists: pgsql-hackers 
On Sun, Apr 5, 2026 at 7:01 PM Andres Freund <andres@anarazel.de> wrote:
> > It's unclear if you're asking (or at least suggesting) that I should
> > invent a new table_index_getnext_slot-like shim, used only during
> > index-only scans, that doesn't require passing a slot (but does
> > require getting a table slot some other way, likely at scan startup).
>
> Why would it require creating a tableam shaped slot internally?  It seems
> rather silly to store a tuple fetched to verify visibility into a slot, where
> it is never used. Storing in a tuple requires an indirect things like
> IncrBufferRefCount(), which isn't that cheap.
>
> heapam_index_getnext_slot(index_only=>1)
> -> heapam_index_fetch_heap_item()
> -> heapam_index_fetch_tuple()

The fact is that nodeIndexOnlyscan.c calls index_fetch_heap right now,
on master, which uses a TupleTableSlot.

Yes, using a slot is suboptimal. But it's nothing new. And this is the
first time I'm hearing about this being a problem. A couple of days
before feature freeze.

> So I guess yes, I am suggesting that we have a separate
> table_index_getnext_ios() or such, which is only usable for index only scans.

What if we offered such an interface but still used a slot internally?
That way we'd have the API you're asking for, without any significant
changes to heap_hot_search_buffer. The caller doesn't need to know
about the table slot's usage.

I get that making the caller use a slot interface with a slot that
they don't exactly need is a bit hokey. We can fix that part, without
seriously risking not getting the patch into 19.

> > Actually, I don't know if we really *should* treat the selfuncs.c
> > caller any different (except as needed to avoid spurious assertion
> > failures).

> I don't think it's worth assuming that that's the code in the core
> infrstructure.

To be clear, I agree.

>         scan->heapRelation->rd_tableam->index_fetch_batch_init(scan, batch, new_alloc);
>
> that's a lot of indirection to go through.
>
>
> Which is what then made me think of embedding something like a
> TableIndexFetchRoutine (with batch_init, reset, markpos, restrpos, end
> members) in IndexScanDesc. That way table_index_{fetch_reset,batch_Init,...}()
> wouldn't need this level of indirection.

But that's the same level of indirection as before? Except it's once
per batch instead of once per tuple.

> Not having the same code in multiple table AMs would make it easier to write
> them, which would be nice in itself, but I am more concerned with ending up
> with copies of heapam_index_getnext_scanbatch_pos() in various AMs, us finding
> problems with heapam_index_getnext_scanbatch_pos() in a minor release, and
> having a harder time adjusting things due to the copied code.

I'll need to study this further.

> > What do you want to do about it? index_getnext_tid isn't adding too
> > much right now, I'd say.
>
> It does indeed not add a whole lot.

> I would move it into a tableam_util_fetch_next_tid() (or such) helper, I
> think.  That also makes it easier to get rid of the table_index_fetch_reset()
> and move the pgstat_count_index_tuples(scan->indexRelation, 1) into one place.

I'll put it into indexbatch.h (next to the function that calls
amgetbatch), and make sure that the amgettuple path is consistent in
terms of how it calls pgstat_count_index_tuples(), and how the
fetch/index scan gets reset.

I don't think that we should be resetting here -- even if it's the
historical behavior. I'm absolutely sure that whatever we do shouldn't
vary based on whether amgetbatch or amgettuple was used (that was not
intended).

> > > Can'tt the *2 lead to computing a xs_vm_items bigger than scan->maxitemsbatch?
> >
> > Yes, it can.
>
> But we prevent it from overflow by a Max()/Min() in the loop headers.

Right.

> > > I'd set scan->batchcache[i] to NULL after freeing.
> >
> > I tried that out, but I *think* it regressed pgbench SELECT by about
> > ~1.5% of total TPS (I'd need more thorough testing to confirm this).
> > Are you sure we need this?
>
> That seems surprising. But no, I don't think we need this.

I'll revisit, might have been wrong.

> It's perhaps ok, I just was a bit surprised to see the callback inside a
> function with that name.  I'd perhaps just name it
> tableam_util_release_batch() (and rename batch_free accordingly).

Will do.

> I'd probably just assert !kill_prior_tuple.

Will do.


-- 
Peter Geoghegan

Commits

  1. read stream: Split decision about look ahead for AIO and combining

  2. read_stream: Only increase read-ahead distance when waiting for IO

  3. aio: io_uring: Trigger async processing for large IOs

  4. heapam: Keep buffer pins across index scan resets.

  5. heapam: Track heap block in IndexFetchHeapData.

  6. Move heapam_handler.c index scan code to new file.

  7. Rename heapam_index_fetch_tuple argument for clarity.

  8. Optimize fast-path FK checks with batched index probes

  9. read_stream: Prevent distance from decaying too quickly

  10. read_stream: Issue IO synchronously while in fast path

  11. bufmgr: Return whether WaitReadBuffers() needed to wait

  12. aio: io_uring: Allow IO methods to check if IO completed in the background

  13. bufmgr: Make UnlockReleaseBuffer() more efficient

  14. Add fake LSN support to hash index AM.

  15. Make IndexScanInstrumentation a pointer in executor scan nodes.

  16. Use fake LSNs to improve nbtree dropPin behavior.

  17. Move fake LSN infrastructure out of GiST.

  18. Use simplehash for backend-private buffer pin refcounts.

  19. nbtree: Avoid allocating _bt_search stack.

  20. bufmgr: Fix use of wrong variable in GetPrivateRefCountEntrySlow()

  21. Conditional locking in pgaio_worker_submit_internal

  22. Reduce ExecSeqScan* code size using pg_assume()

  23. Fix rare bug in read_stream.c's split IO handling.

  24. Remove HeapBitmapScan's skip_fetch optimization

  25. Optimize nbtree backwards scans.

  26. Fix multiranges to behave more like dependent types.

  27. Add EXPLAIN (MEMORY) to report planner memory consumption

  28. Optimize nbtree backward scan boundary cases.

  29. Increment xactCompletionCount during subtransaction abort.

  30. Add nbtree Valgrind buffer lock checks.

  31. Add nbtree high key "continuescan" optimization.

  32. Reduce pinning and buffer content locking for btree scans.

  33. Teach btree to handle ScalarArrayOpExpr quals natively.