Re: Add os_page_num to pg_buffercache

Hi,

On Wed, Jul 09, 2025 at 10:51:16AM +0100, Mircea Cadariu wrote:
> If you don't mind I have some further questions on the patch, see below.

Thanks for the feedback/questions!

> > +        if (get_call_result_type(fcinfo, NULL, &expected_tupledesc) != TYPEFUNC_COMPOSITE)
> > +            elog(ERROR, "return type must be a row type");
> 
> Is this needed in the new pg_buffercache_os_pages function?

Strictly speaking it is not, we could CreateTemplateTupleDesc(NUM_BUFFERCACHE_OS_PAGES_ELEM)
instead of CreateTemplateTupleDesc(expected_tupledesc->natts). OTOH, it's used
in multiple places in this extension so I think it's ok to keep it that way
for consistency.

> I noticed this
> check also in the "original" pg_buffercache_pages. There's a comment there
> indicating that (if I understand correctly) its purpose is to handle
> upgrades from version 1.0, mentioning a field unrelated to this patch.
> 
> If it's needed, shall we consider adding a similar comment as
> in pg_buffercache_pages?

We don't need the same kind of comment in pg_buffercache_os_pages() because
it's new in 1.7 (so the patch can not "break" a pre-1.7 version of this function
/view).

In the pg_buffercache_pages case, the story is different, it's used to deal
with the pinning_backends fields that has been introduced in 1.1 (see
pg_buffercache--1.0--1.1.sql).

> > +        /*
> > +         * Different database block sizes (4kB, 8kB, ..., 32kB) can be used,
> > +         * while the OS may have different memory page sizes.
> > +         *
> > +         * To correctly map between them, we need to: 1. Determine the OS
> > +         * memory page size 2. Calculate how many OS pages are used by all
> > +         * buffer blocks 3. Calculate how many OS pages are contained within
> > +         * each database block.
> > +         */
> For step number 3 - should it be the other way around: database blocks are
> contained within OS pages?

This comment comes from pg_get_shmem_allocations_numa() and I agree that it
could be misleading: it all depends what the OS and block sizes actually are:
fixed in v5 attached where the wording is almost the same as in 
pg_buffercache_numa_pages().

Also I think that it is not correct in pg_get_shmem_allocations_numa(), I think
that it should be something like proposed in [1].

[1]: https://www.postgresql.org/message-id/aH4DDhdiG9Gi0rG7%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com