On Sun, Mar 15, 2026 at 3:49 PM Tomas Vondra <tomas@vondra.me> wrote:
>
> - If the separation between TAM and the low-level instrumentation clear
> enough? Or is the ReadStreamInstrumentation "leaking" somewhere? For
> example, is it OK it's in SeqScanInstrumentation?
Personally, I don't like having both structs
(ReadStreamInstrumentation and TableScanStatsData).
The executor nodes (SeqScanState, BitmapHeapScanState) already embed
ReadStreamInstrumentation directly in their instrumentation structs,
so we already have a reference to the read stream in table AM-agnostic
code. Having a second identical struct means maintaining two
definitions without any actual benefit.
> But I'm sure there are other questions I haven't thought of.
I see a couple more issues with the counting in read_stream.c.
You are double-counting stalls for synchronous IO. You increment
stalls in read_stream_next_buffer() but we actually execute
synchronous IO in WaitReadBuffers and return needed_wait as true,
which will count a stall again.
You are not counting fast path IOs because those don't go through
read_stream_start_pending_read() and instead are started directly by
StartReadBuffer() in read_stream_next_buffer(). Simple diff attached
should fix this.
Also, per worker stats are not displayed when BUFFERS false is passed
even with IO true because of a small oversight. I fixed it in the
attached diff.
- Melanie
