On Tue, Mar 31, 2026 at 12:02 PM Andres Freund <[email protected]> wrote: > > 0001+0002: Return whether WaitReadBuffers() needed to wait > > The first patch allows pgaio_wref_check_done() to work more reliably with > io_uring. Until now it only was able to return true if userspace already > had consumed the kernel's completion event, but returned false otherwise. > That's not really incorrect, just suboptimal. > > The second patch returns whether WaitReadBuffers() needed to wait for > IO. This is useful for a) instrumentation like in [2] and b) to provide > information to the read_stream heuristics to control how aggressive to > perform read ahead.
These both look good to me except in 0001 you left an XXX in pgaio_wref_check_done() that I think is the very thing that commit does. > 0003: read_stream: Issue IO synchronously while in fast path LGTM > 0004: read_stream: Prevent distance from decaying too quickly > > There are two minor questions here: > - Should read_stream_pause()/read_stream_resume() restore the "holdoff" > counter? I doubt it matters for the prospective user, since it will > only be used when the lookahead distance is very large. I don't really understand this. We have to do this with distance because we set it to 0 and use distance == 0 to indicate stream end. read_stream_pause() doesn't set the distance_decay_holoff to 0. If you mean, should we reset holdoff to its initial value, then I don't think so. I imagine that users doing a lot of pause and resume may not have high distance. > - For how long to hold off distance reductions? Initially I was torn > between using "max_pinned_buffers" (Min(max_ios * io_combine_limit, > cap)) and "max_ios" ([maintenance_]effective_io_concurrency). But I > think the former makes more sense, as we otherwise won't allow for far > enough readahead when doing IO combining, and it does seem to make sense > to hold off decay for long enough that the maximum lookahead could not > theoretically allow us to start an IO. I agree. 0004 LGTM otherwise. - Melanie
