Re: Draft for basic NUMA observability - Mailing list pgsql-hackers

On Tue, Apr 1, 2025 at 5:13 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi Jakub,
>
> On Tue, Apr 01, 2025 at 12:56:06PM +0200, Jakub Wartak wrote:
> > On Mon, Mar 31, 2025 at 4:59 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> >
> > > Hi,
> >
> > Hi Bertrand, happy to see you back, thanks for review and here's v18
> > attached (an ideal fit for PG18 ;))
>
> Thanks for the new version!
>

Hi Bertrand,

I'll be attaching v20 when responding to the follow-up review by Tomas
to avoid double-sending this in the list, but review findings from
here are fixed in v20.

> === About v18-0002
>
> It looks in a good shape to me. The helper's name might still be debatable
> though.
>
> I just have 2 comments:
>
> === 1
>
> +       if (bufRecord->blocknum == InvalidBlockNumber ||
> +               bufRecord->isvalid == false)
>
> It seems to me that this check could now fit in one line.

OK.

> === 2
>
> +       {
>                 SRF_RETURN_DONE(funcctx);
> +       }
>
> Extra parentheses are not needed.

This also should be fixed in v20.

>
> === About v18-0003
>
> === 3
>
> I think that pg_buffercache--1.5--1.6.sql is not correct. It should contain
> only the necessary changes when updating from 1.5. It means that it should "only"
> create the new objects (views and functions in our case) that come in v18-0003
> and grant appropriate privs.
>
> Also it should mention:
>
> "
> \echo Use "ALTER EXTENSION pg_buffercache UPDATE TO '1.6'" to load this file. \quit
> "
> and not:
>
> "
> +\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
> "
>
> The already existing pg_buffercache--1.N--1.(N+1).sql are good examples.

Hm, good find, it might be leftover from first attempts where we were
aiming just for adding column numa_node_id to pg_buffercache_pages()
(rather than adding new view), all hopefully fixed.

> === 4
>
> +               for (i = 0; i < NBuffers; i++)
> +               {
> +                       int                     blk2page = (int) i * pages_per_blk;
> +
>
> I think that should be:
>
> int blk2page = (int) (i * pages_per_blk);

OK, but I still fail to grasp why pg_indent doesnt fix this stuff on
it's own... I believe orginal ident, would fix this on it's own?

> === About v18-0004
>
> === 5
>
> When running:
>
> select c.name, c.size as num_size, s.size as shmem_size
> from (select n.name as name, sum(n.size) as size from pg_shmem_numa_allocations n group by n.name) c,
pg_shmem_allocationss 
> where c.name = s.name;
>
> I can see:
>
> - pg_shmem_numa_allocations reporting a lot of times the same size
> - pg_shmem_numa_allocations and pg_shmem_allocations not reporting the same size
>
> Do you observe the same?
>

Yes, it is actually by design: the pg_shmem_allocations.size is sum of
page sizes not size of struct, e.g. with "order by 3 desc":

                      name                      | num_size  | shmem_size
------------------------------------------------+-----------+------------
 WaitEventCustomCounterData                     |      4096 |          8
 Archiver Data                                  |      4096 |          8
 SerialControlData                              |      4096 |         16

I was even wondering if it does make sense to output "shm pointer
address" (or at least offset) there to see which shm structures are on
the same page, but we have the same pg_shm_allocations already.

-J.