Re: EXPLAIN: showing ReadStream / prefetch stats

Hi,

On 2026-03-30 20:21:29 +0200, Tomas Vondra wrote:
> >> From 410eaaebe7b814ac9f44c080e153f4ec1d6d6b86 Mon Sep 17 00:00:00 2001
> >> From: Tomas Vondra <tomas(at)vondra(dot)me>
> >> Date: Thu, 19 Mar 2026 22:25:09 +0100
> >> Subject: [PATCH v5 3/6] explain: show prefetch stats in EXPLAIN (ANALYZE)
> >>
> >> This adds details about AIO / prefetch for executor nodes using a
> >> ReadStream. Right now this applies only to BitmapHeapScan, because
> >> that's the only scan node using a ReadStream and collecting
> >> instrumentation from workers.
> >
> > I don't understand why that means it should be done as part of this commit,
> > whereas seqscans shouldn't?
> >
>
> Are you suggesting the commit adds support for all those scans (BHS,
> SeqScan and TidRangeScan) or none of them?

I guess I mostly just didn't quite understand what differentiates bitmap scans
from the other scans, based on this explanation.

> To me it seems better to have at least some scan because of testing. But
> SeqScan/TidRangeScan don't have the instrumentation infrastructure for
> parallel queries, and I don't want to do that in the main patch - it seems
> rather unrelated. And I also don't want to add it before the main patch.

I'd probably do the latter, i.e. add it before the main patch. Or at least
separately from the change to show read stream instrumentation.

> > Separately, is %.3f really the right thing? When is that degree of precision
> > useful here?
> >
>
> No idea. I think %.1f would be enough. We use %.2f in a couple places,
> but that's not a huge difference.

I'd probably use .1f here, I don't think you'd ever need more
precision. Showing that it's not a round number is useful, but more than
that... ?

> >> /* ----------------------------------------------------------------
> >> @@ -404,9 +471,50 @@ void
> >> ExecSeqScanInitializeWorker(SeqScanState *node,
> >> ParallelWorkerContext *pwcxt)
> >> {
> >> + EState *estate = node->ss.ps.state;
> >> ParallelTableScanDesc pscan;
> >> + char *ptr;
> >> + Size size;
> >>
> >> pscan = shm_toc_lookup(pwcxt->toc, node->ss.ps.plan->plan_node_id, false);
> >> node->ss.ss_currentScanDesc =
> >> - table_beginscan_parallel(node->ss.ss_currentRelation, 0, pscan);
> >> + table_beginscan_parallel(node->ss.ss_currentRelation,
> >> + (estate->es_instrument) ? SO_SCAN_INSTRUMENT : 0,
> >> + pscan);
> >> +
> >> + /*
> >> + * Workers don't get the pscan_len value in scan descriptor, so use the
> >> + * TAM callback again. The result has to match the earlier result in
> >> + * ExecSeqScanEstimate.
> >> + */
> >> + size = table_parallelscan_estimate(node->ss.ss_currentRelation,
> >> + estate->es_snapshot);
> >> +
> >
> > That seems quite grotty...
> >
>
> Yes. But what's a better solution?

Think the way you've done it in v6 is better.

Greetings,

Andres Freund