Re: EXPLAIN: showing ReadStream / prefetch stats

On 4/7/26 18:07, Melanie Plageman wrote:
> On Tue, Apr 7, 2026 at 8:36 AM Tomas Vondra <tomas@vondra.me> wrote:
>>
>> I've pushed the first two parts earlier today. Here's a v13 with the
>> remaining pieces rebased on top of that, with updated commit messages
>> and some additional fixes.
> 
> I noticed a couple of things while reviewing 0004.
> 
> I see I had bitmapheapscan use
>     if (!node->ss.ps.instrument || pcxt->nworkers == 0)
>         return;
> 
> while seq scan uses
>     if (!estate->es_instrument || pcxt->nworkers == 0)
>         return;
> 
> That does seem worth being consistent about. Though the estate one is
> probably better to use and changing bitmapheapscan in this commit
> might be noisy... I don't feel strongly either way.
> 

I'm not sure this is just a question of consistency, because BHS may
need the shared instrumentation even if (es_instrument = 0), no? While
the seqscan/tidrange scan only need it with EXPLAIN(IO).

So I think the two nodes should check

  ((estate->es_instrument & INSTRUMENT_IO) == 0)

The attached v14 does it this way. I left BHS to check ps.instrument, I
think that's fine.

> In ExecSeqScanInstrumentInitWorker() -> shm_toc_lookup() noerror is true:
> 
>     node->sinstrument = shm_toc_lookup(pwcxt->toc,
>                                        node->ss.ps.plan->plan_node_id
> + PARALLEL_KEY_SCAN_INSTRUMENT_OFFSET,
>                                        true);
> 
> I went back and forth on this for bitmapheapscan and ended up going
> with the guard
>     if (!node->ss.ps.instrument)
>         return;
> and passing noerror as false. I thought it would be better to get an
> error if something went wrong.
> 
> If you keep noerror true, there's no reason to check for es_instrument.

I agree this should use noerror=false. It's an error to not find the
expected DSM chunk, and we don't want to hide that. It'd probably fail
with a segfault later, but that's hardly a better outcome.

> 
> I also noticed that seq scan doesn't use add_size/mul_size for the
> size calculations when estimating and allocating DSM. I switched to
> using them in bitmapheapscan because I realized other code in that
> file used them. Seems seq scan was doing plain arithmetic without
> those helpers already. And overflow is unlikely here. But I wonder if
> it is worth using them?
> 

Seems better to use that, so done in v14.

> It doesn't matter for the Retrieve functions because overflow would
> have caused us to error out at estimation or allocation time.
> 
> Most of the seq scan specific stuff above applies to tid range scan as well.
> 

OK, fixed that too.

> One other point about 0002: I wonder if the text explain output should
> say "in-progress" instead of "inprogress"
> 

True. Changed.



Thanks!

-- 
Tomas Vondra