MIME-Version: 1.0
References: 
 <CAP53Pkx0=ph0vG_M20yVAoK11yGSTZP=53-rZt36OCP4hBPaDQ@mail.gmail.com>
 <CAD21AoCKz0t06-qOOm_7kgMn=Niu0yYB3R5jhOY0oVb=5AWytA@mail.gmail.com>
 <CAExHW5uqQuPNEjj2ondemCYdtFHExm=jvw-4naPn7gdMmkhrgw@mail.gmail.com>
 <CAP53PkyK65PM-Ws7ctj5-eKqVxHo5FzoWKW7=XPq5Jc1qZCJNg@mail.gmail.com>
 <CAD21AoCzv5q89VDZ=Ot1wWguBO0MGdCc0-NXN83-Jg57uiJN1Q@mail.gmail.com>
 <CAExHW5tynt3Vmmhvvk929Ljfz6jTaJWWoRDthhV=e13GvYRSqQ@mail.gmail.com>
 <670b867e-6518-48b0-b7c2-bd5f10ab58c6@vondra.me>
In-Reply-To: <670b867e-6518-48b0-b7c2-bd5f10ab58c6@vondra.me>
From: Lukas Fittl <lukas@fittl.com>
Date: Sat, 28 Mar 2026 12:38:04 -0700
Message-ID: 
 <CAP53PkzP=MEXFvCgEHZbZqxm5C69L5ig3c4qdPxU82=+H8C2iw@mail.gmail.com>
Subject: Re: pg_buffercache: Add per-relation summary stats
To: Tomas Vondra <tomas@vondra.me>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Paul A Jungwirth <pj@illuminatedcomputing.com>,
 Khoa Nguyen <khoaduynguyen@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAP53PkzP%3DMEXFvCgEHZbZqxm5C69L5ig3c4qdPxU82%3D%2BH8C2iw%40mail.gmail.com>
Precedence: bulk

Hi Tomas,

Thanks for reviewing - just a quick response on your code review
comments specifically:

On Fri, Mar 27, 2026 at 3:58=E2=80=AFPM Tomas Vondra <tomas@vondra.me> wrot=
e:
> 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 =3D=3D 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