Hi,
On 2025-07-16 15:39:58 -0400, Andres Freund wrote:
> Looking at the actual patches now.
I just did an initial, not particularly in depth look. A few comments and
questions below.
For either patch, I think it's high time we split the index/table buffer stats
in index scans. It's really annoying to not be able to see if IO time was
inside the index itself or in the table. What we're discussing here obviously
can never avoid stalls due to fetching index pages, but so far neither patch
is able to fully utilize hardware when bound on heap fetches, but that's
harder to know without those stats.
The BufferMatches() both patches add seems to check more than needed? It's not
like the old buffer could have changed what relation it is for while pinned.
Seems like it'd be better to just keep track what the prior block was and not
go into bufmgr.c at all.
WRT the complex patch:
Maybe I'm missing something, but the current interface doesn't seem to work
for AMs that don't have a 1:1 mapping between the block number portion of the
tid and the actual block number?
Currently the API wouldn't easily allow the table AM to do batched TID lookups
- if you have a query that looks at a lot of table tuples in the same buffer
consecutively, we spend a lot of time locking/unlocking said buffer. We also
spend a lot of time dispatching from nodeIndexscan.c to tableam in such
queries.
I'm not suggesting to increase the scope to handle that, but it might be worth
keeping in mind.
I think the potential gains here are really substantial. Even just not having
to lock/unlock the heap block for every tuple in the page would be a huge win,
a quick and incorrect hack suggests it's like 25% faster A batched
heap_hot_search_buffer() could be a larger improvement, it's often bound by
memory latency and per-call overhead.
I see some slowdown for well-cached queries with the patch, I've not dug into
why.
WRT the simple patch:
Seems to have the same issue that it assumes TID block numbers correspond to
actual disk location?
Greetings,
Andres Freund
