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

Hi,

On Mon, Mar 31, 2025 at 11:27:50AM +0200, Jakub Wartak wrote:
> On Thu, Mar 27, 2025 at 2:40 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > > +Size
> > > +pg_numa_get_pagesize(void)
> [..]
> >
> > Should this have a comment or an assertion that it can only be used after
> > shared memory startup? Because before that huge_pages_status won't be
> > meaningful?
> 
> Added both.

Thanks for the updated version!

+       Assert(IsUnderPostmaster);

I wonder if that would make more sense to add an assertion on huge_pages_status
and HUGE_PAGES_UNKNOWN instead (more or less as it is done in
CreateSharedMemoryAndSemaphores()).

=== About v17-0002-This-extracts-code-from-contrib-pg_buffercache-s.patch

Once applied I can see mention to pg_buffercache_numa_pages() while it
only comes in v17-0003-Extend-pg_buffercache-with-new-view-pg_buffercac.patch.

I think pg_buffercache_numa_pages() should not be mentioned before it's actually
implemented.

=== 1

+               bufRecord->isvalid == false)
        {
                int                     i;

-               funcctx = SRF_FIRSTCALL_INIT();
-
-               /* Switch context when allocating stuff to be used in later calls */
-               oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
-
-               /* Create a user function context for cross-call persistence */
-               fctx = (BufferCachePagesContext *) palloc(sizeof(BufferCachePagesContext));
+               for (i = 1; i <= 9; i++)
+                       nulls[i] = true; 

"i <= 9" will be correct only once v17-0003 is applied (when NUM_BUFFERCACHE_PAGES_ELEM
is increased to 10).

In v17-0002 that should be i < 9 (even better i < NUM_BUFFERCACHE_PAGES_ELEM).

That could also make sense to remove the loop and use memset() that way:

"
memset(&nulls[1], true, (NUM_BUFFERCACHE_PAGES_ELEM - 1) * sizeof(bool));
"

instead. It's done that way in some other places (hbafuncs.c for example).

=== 2

-               if (expected_tupledesc->natts == NUM_BUFFERCACHE_PAGES_ELEM)
-                       TupleDescInitEntry(tupledesc, (AttrNumber) 9, "pinning_backends",
-                                                          INT4OID, -1, 0);

+       if (expected_tupledesc->natts >= NUM_BUFFERCACHE_PAGES_ELEM - 1)
+               TupleDescInitEntry(tupledesc, (AttrNumber) 9, "pinning_backends",
+                                                  INT4OID, -1, 0);

I think we should not change the "expected_tupledesc->natts" check here until
v17-0003 is applied.

Regards,

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