Re: Draft for basic NUMA observability

Hi,

On Thu, Mar 13, 2025 at 02:15:14PM +0000, Bertrand Drouvot wrote:
> > > === 19
> > >
> > 
> > Can you please take a look again on this
> 
> Sure, will do.


> I'll have a look at v11 soon.


About 0001:

=== 1

git am produces:

.git/rebase-apply/patch:378: new blank line at EOF.
+
.git/rebase-apply/patch:411: new blank line at EOF.
+
warning: 2 lines add whitespace errors.

=== 2

+  Returns true if a <acronym>NUMA</acronym> support is available.

What about "Returns true if the server has been compiled with NUMA support"?

=== 3

+Datum
+pg_numa_available(PG_FUNCTION_ARGS)
+{
+       if(pg_numa_init() == -1)
+               PG_RETURN_BOOL(false);
+       PG_RETURN_BOOL(true);
+}

What about PG_RETURN_BOOL(pg_numa_init() != -1)?

Also I wonder if pg_numa.c would not be a better place for it.

=== 4

+{ oid => '5102', descr => 'Is NUMA compilation available?',
+  proname => 'pg_numa_available', provolatile => 'v', prorettype => 'bool',
+  proargtypes => '', prosrc => 'pg_numa_available' },
+

Not sure that 5102 is a good choice.

src/include/catalog/unused_oids is telling me:

Best practice is to start with a random choice in the range 8000-9999.
Suggested random unused OID: 9685 (23 consecutive OID(s) available starting here)

So maybe use 9685 instead?

=== 5

@@ -12477,3 +12481,4 @@
   prosrc => 'gist_stratnum_common' },

 ]
+

garbage?

=== 6

Run pgindent as it looks like it's finding some things to do in src/backend/storage/ipc/shmem.c
and src/port/pg_numa.c.

=== 7

+Size
+pg_numa_get_pagesize(void)
+{
+       Size os_page_size = sysconf(_SC_PAGESIZE);
+       if (huge_pages_status == HUGE_PAGES_ON)
+                GetHugePageSize(&os_page_size, NULL);
+       return os_page_size;
+}

I think that's more usual to:

Size
pg_numa_get_pagesize(void)
{
       Size os_page_size = sysconf(_SC_PAGESIZE);

       if (huge_pages_status == HUGE_PAGES_ON)
                GetHugePageSize(&os_page_size, NULL);

       return os_page_size;
}

I think that makes sense to check huge_pages_status as you did.

=== 8

+extern void numa_warn(int num, char *fmt,...) pg_attribute_printf(2, 3);
+extern void numa_error(char *where);

I wonder if that would make sense to add a comment mentioning
github.com/numactl/numactl/blob/master/libnuma.c here.


I still need to look at 0002 and 0003.

Regards,

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