On 4/7/26 17:27, Melanie Plageman wrote:
> On Tue, Apr 7, 2026 at 8:36 AM Tomas Vondra <tomas@vondra.me> wrote:
>>
>> 3) Used (es_instrument & INSTRUMENT_IO) more consistently. A couple
>> places in the executor still checked just es_instrument, and so would
>> collect stats even if not needed. Be consistent.
>
> I would argue for only allocating the shared instrumentation if they
> will be displayed, but I don't feel that strongly about it.
>
OK
> I took another look at just 0002 just to double-check the read stream
> counters. It seems fine.
> Perhaps we should free the TableScanInstrumentation in heap_endscan().
> It isn't that important of a leak, but the other things palloc'd in
> heap_beginscan() are freed.
>
Good point, will fix.
> I also wondered if it is clear without a comment why we don't count a
> wait when READ_BUFFERS_SYNCHRONOUSLY sets needed_wait (which we added
> to make sure distance ramps up). I think it's fine; I'm just musing.
>
I believe this is actually a bug, the read_stream_count_wait() should be
after the block checking for READ_BUFFERS_SYNCHRONOUSLY. I recall Andres
mentioned this in one of this reviews, but I either forgot to address
that, or it got lost then transferring this between this thread and the
index prefetching one.
Thanks!
--
Tomas Vondra
