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 <tomas@vondra.me> 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
