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
