On Sun, Mar 15, 2026 at 3:49 PM Tomas Vondra <[email protected]> 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
From 176578c63fc2833157646f668bd0a007253fc5aa Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Mon, 16 Mar 2026 13:10:46 -0400 Subject: [PATCH] fixups --- src/backend/commands/explain.c | 2 +- src/backend/storage/aio/read_stream.c | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 41c9cabd9ae..5491b56d12b 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -2306,7 +2306,7 @@ ExplainNode(PlanState *planstate, List *ancestors, show_wal_usage(es, &planstate->instrument->walusage); /* Prepare per-worker buffer/WAL usage */ - if (es->workers_state && (es->buffers || es->wal) && es->verbose) + if (es->workers_state && (es->buffers || es->wal || es->io) && es->verbose) { WorkerInstrumentation *w = planstate->worker_instrument; diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index 627afafef05..1227721ab5b 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -904,9 +904,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) stream->ios_in_progress = 1; stream->ios[0].buffer_index = oldest_buffer_index; stream->seq_blocknum = next_blocknum + 1; - - /* since we executed IO synchronously, count it as a stall */ - stream->stats.stall_count += 1; + read_stream_update_stats_io(stream, 1, stream->ios_in_progress); } else { -- 2.43.0
