Hi Haibo,

Thanks for your review!

On Mon, Mar 16, 2026 at 9:21 PM Haibo Yan <[email protected]> wrote:
> Could this use RelFileLocator plus ForkNumber instead of open-coding 
> BufferRelStatsKey? That seems closer to existing PostgreSQL abstractions for 
> physical relation identity.

Yes, that was noted by other reviewers as well, and makes sense.

> I wonder whether pg_buffercache_relation_stats() is the best name here. The 
> function is really aggregating by relation file identity plus fork, and it is 
> producing a summary of the current buffer contents rather than what many 
> readers might assume from “relation stats”. Would something with summary be 
> clearer than stats?

Per the most recent feedback, I'll rename this to
"pg_buffercache_relations" for now.

> Why are OUT relforknumber and OUT relfilenode exposed as int2 and oid 
> respectively? Internally these are represented as ForkNumber and 
> RelFileNumber, so I wonder whether the SQL interface should reflect that more 
> clearly, or at least whether the current choice should be explained.

This is consistent with how pg_buffercache_pages represents them - I
think those are the correct mappings of the int ForkNumber (which we
know to be small in practice) and RelFileNumber is a typedef of Oid.

> The comment says, “Hash key for pg_buffercache_relation_stats — groups by 
> relation identity”, but that seems imprecise. It is really grouping by 
> relfilenode plus fork, i.e. physical relation-file identity rather than 
> relation identity in a more logical sense.

Good point. I'll adapt this to "groups by relation file" for now.

> Is PARALLEL SAFE actually desirable here, as opposed to merely technically 
> safe? A parallel query could cause multiple workers to perform full 
> shared-buffer scans independently, which does not seem obviously desirable 
> for this kind of diagnostic function.

I see your point, but I don't think a parallel plan would happen in
practice when just the function is being queried. Since other
pg_buffercache functions are also PARALLEL SAFE, I'll keep this as is
for now - if we want to adjust it we should be consistent I think.

Thanks,
Lukas

-- 
Lukas Fittl


Reply via email to