Re: EXPLAIN: showing ReadStream / prefetch stats

On 3/16/26 18:14, Melanie Plageman wrote:
> 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.
> 

That's a good point. The executor nodes should not embed anything
readstream-specific, not even in the instrumentation part. AFAICS the
cleanest way would be to replace this with the TableScanStatsData.

However, ReadStreamInstrumentation is not really ReadStream specific,
except for the name. I'm actually wondering if we should have just one
struct, that'd be enough.

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

Thanks!

-- 
Tomas Vondra