| From: | Lukas Fittl <lukas(at)fittl(dot)com> |
|---|---|
| To: | Tomas Vondra <tomas(at)vondra(dot)me> |
| Cc: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com>, Khoa Nguyen <khoaduynguyen(at)gmail(dot)com> |
| Subject: | Re: pg_buffercache: Add per-relation summary stats |
| Date: | 2026-03-28 19:38:04 |
| Message-ID: | CAP53PkzP=MEXFvCgEHZbZqxm5C69L5ig3c4qdPxU82=+H8C2iw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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(at)vondra(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tom Lane | 2026-03-28 19:46:01 | Re: astreamer fixes |
| Previous Message | Mihail Nikalayeu | 2026-03-28 19:17:00 | Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements |