Re: EXPLAIN: showing ReadStream / prefetch stats

On 4/6/26 00:18, Melanie Plageman wrote:
> On Fri, Apr 3, 2026 at 3:01 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>>
>> 1) stats for non-parallel-aware scans
>>
>> It correctly shows stats for scans that are not parallel-aware.
> <--snip-->
>> For SeqScan/TidRangeScan I've merged the changes into 0004 and 0005.
>>
>> I have to admit I'm not a huge fan of the changes required to fix this.
>> The logic with all these instument/parallel_aware conditions seems much
>> less readable to me, but I don't know how else to do this. The approach
>> is copied from nodeIndexscan.c, which already did that in 0fbceae841cb.
>
> While this is the opposite direction of what I suggested to fix BHS in
> [1], what if we allocated the instrumentation and parallel-aware state
> separately and accessed them with their own keys? It's a little janky
> because what key could we use besides the plan_node_id, but if we add
> a key-sized offset to the plan node id, we can functionally have two
> separate keys.
>

Interesting idea! That'd mean we don't need to mess with the
instrument/parallel_aware flags in the functions. That seems like an
improvement, it made the code ... not nice.

Presumably, we'd only do this in master? It seems way too invasive to
backpatch (and for index scans it'd even be an ABI break, so we can't do
that). Moreover, for index scans it's not even a bug, and it does not
seem great to do it one way for BHS and a completely different way in
index scans.

So we'd either not fix BHS in backbranches, or do it the "ugly" way.

> We could do this for all nodes that have both shared instrumentation
> and parallel-aware shared state. It introduces some boilerplate
> functions, but I do find it easier to understand.
>
> I've attached patches that implement this to make the idea more clear.
> 0002 does the two allocations for the existing index[-only] scan
> nodes. 0003 uses this method to fix BHS. 0004 is your patch to add
> prefetch stuff to explain and show it for BHS. And 0005 and 0006 are
> using the two allocation approach for seqscan and tidrange scan. There
> were some test changes you had that didn't seem required to make tests
> pass in the commits that I stuffed into 0007. I used an LLM to do some
> of the boilerplate and rebasing, so there might be some weirdness in
> there.

Thanks! It's a bit late for me to experiment with this, but I'll take a
closer look tomorrow. We're getting pretty clear to feature freeze, and
these seem like big changes ...

>
> If we don't do the above, then I think your current approach is the
> only other realistic option. We can't do what I suggested for BHS in
> [1] and always allocate the parallel-aware state because that state is
> much larger for sequential scans and TID range scans.
>

Yeah. I was wondering about these costs when you proposed to allocate
the BHS parallel state always. I concluded it does not matter for BHS,
but for scans with larger states it might be different.

regards

--
Tomas Vondra