Hi Tomas, Thanks for reviewing - just a quick response on your code review comments specifically:
On Fri, Mar 27, 2026 at 3:58 PM Tomas Vondra <[email protected]> wrote: > I gave it a try on an azure VM with 32GB shared buffers, to make it a > bit more realistic, and my timings are 10ms vs. 700ms. But I also wonder > if the original timings really were from a cluster with 128MB, because > for me that shows 0.3ms vs. 3ms (so an order of magnitude faster than > what was reported). But I suppose that's also hw specific. Yeah, those initial numbers were from my Apple Silicon M3 ARM laptop without any special configuration, just for reference. > A couple minor comments about the code: > > 1) Isn't this check unnecessary? All entries should have buffers > 0. > > if (entry->buffers == 0) > continue; Yeah, good point, that is there to protect the division for the avg usage count, but I agree in practice this shouldn't be reached. I could make it an assert, just in case. > 2) Shouldn't this check BM_TAG_VALID too? Or is BM_VALID enough to look > at the bufHdr->tag? > > /* Skip unused/invalid buffers */ > if (!(buf_state & BM_VALID)) > continue; > Good point, I think that makes sense to check BM_TAG_VALID here as well. FWIW, the function as-is does not lock the buffer header with LockBufHdr (intentionally to lower overhead), which means we can read a stale relation reference. I think that's okay for aggregate level / monitoring type information, but just want to call it out in this context. > 3) I think "buffers" argument should be renamed to "buffers_unused" for > consistency with pg_buffercache_summary. I assume you meant "buffers_used" instead of "buffers_unused" - assuming yes, that makes sense for consistency. --- I'll hold off on posting a new version for now, since I think we'd have to figure out a solution to the work_mem question at the very least, and it sounds like right now its also a toss-up in terms of overall interest to get this committed. Thanks, Lukas -- Lukas Fittl
