Re: EXPLAIN: showing ReadStream / prefetch stats

On 3/31/26 19:41, Andres Freund wrote:
> 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.
>

Sorry, I'm confused. Which "latter" option you mean?

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

OK, .1f WFM

>
>>>> /* ----------------------------------------------------------------
>>>> @@ -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.
>

Thanks, I agree. I'll adjust the table_parallelscan_initialize to take
the length as an extra argument.

regards

--
Tomas Vondra