Re: EXPLAIN: showing ReadStream / prefetch stats

On Tue, Apr 7, 2026 at 8:36 AM Tomas Vondra <tomas(at)vondra(dot)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.

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

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.

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

- Melanie