Hi,
On 2026-04-07 18:13:18 +0200, Tomas Vondra wrote:
> On 4/7/26 17:27, Melanie Plageman wrote:
> > 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.
Yea, I think it's actually somewhat important that those will be counted as
waits, since they *are* blocking reads. It's just that the blocking read
happends during the start of the IO, so we don't have to wait in
WaitReadBuffers(). You'd otherwise severely undercounting reads in cases where
the distance ramps up and down a lot
Greetings,
Andres Freund
