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

On 4/9/25 17:14, Andres Freund wrote:
> Hi,
> 
> On 2025-04-09 16:33:14 +0200, Tomas Vondra wrote:
>> From e1f093d091610d70fba72b2848f25ff44899ea8e Mon Sep 17 00:00:00 2001
>> From: Tomas Vondra <tomas@vondra.me>
>> Date: Tue, 8 Apr 2025 23:31:29 +0200
>> Subject: [PATCH 1/2] Cleanup of pg_numa.c
>>
>> This moves/renames some of the functions defined in pg_numa.c:
>>
>> * pg_numa_get_pagesize() is renamed to pg_get_shmem_pagesize(), and
>>   moved to src/backend/storage/ipc/shmem.c. The new name better reflects
>>   that the page size is not related to NUMA, and it's specifically about
>>   the page size used for the main shared memory segment.
>>
>> * move pg_numa_available() to src/backend/storage/ipc/shmem.c, i.e. into
>>   the backend (which more appropriate for functions callable from SQL).
>>   While at it, improve the comment to explain what page size it returns.
>>
>> * remove unnecessary includes from src/port/pg_numa.c, adding
>>   unnecessary dependencies (src/port should be suitable for frontent).
>>   These were leftovers from earlier patch versions.
> 
> I don't think the include in src/port/pg_numa.c were leftover? Just the one in
> pg_numa.h, right?
> 

Right, that wasn't quite accurate. The miscadmin.h and pg_shmem.h are
unnecessary thanks to moving stuff to shmem.c.

> I'd mention that the includes of postgres.h/fmgr.h is what caused missing
> build-time dependencies and via that failures on buildfarm member dogfish.
> 

Not really, I also need to include "c.h" instead of "postgres.h" (which
is also causing the same failure).

> 
>> diff --git a/src/port/pg_numa.c b/src/port/pg_numa.c
>> index 5e2523cf798..63dff799436 100644
>> --- a/src/port/pg_numa.c
>> +++ b/src/port/pg_numa.c
>> @@ -13,17 +13,14 @@
>>   *-------------------------------------------------------------------------
>>   */
>>  
>> -#include "postgres.h"
>> +#include "c.h"
>>  #include <unistd.h>
>>  
>>  #ifdef WIN32
>>  #include <windows.h>
>>  #endif
> 
> I think this may not be needed anymore, that was just there for
> GetSystemInfo(), right?  Conversely, I suspect it may now be needed in the new
> location of pg_numa_get_pagesize()?
> 

Good question. But if it's needed there, shouldn't it have failed on CI?

> 
>> From 201f8be652e9344dfa247b035a66e52025afa149 Mon Sep 17 00:00:00 2001
>> From: Tomas Vondra <tomas@vondra.me>
>> Date: Wed, 9 Apr 2025 13:29:31 +0200
>> Subject: [PATCH 2/2] ci: Check for missing dependencies in meson build
>>
>> Extends the meson build on Debian to also check for missing dependencies
>> by executing
>>
>>     ninja -t missingdeps
>>
>> right after the build. This highlights unindended dependencies.
>>
>> Reviewed-by: Andres Freund <andres@anarazel.de>
>> https://postgr.es/m/CALdSSPi5fj0a7UG7Fmw2cUD1uWuckU_e8dJ+6x-bJEokcSXzqA@mail.gmail.com
> 
> FWIW, while I'd prefer it as a meson.build visible test(), I think it's ok to
> have it just in CI until we have that.  I would however also add it to the
> windows job, as that's the most "different" type of build / source of missed
> dependencies that wouldn't show up on our development systems.
> 

We can add it as a meson.build test, sure. I was going for the CI first,
because then it fires no matter what build I do locally (I'm kinda still
used to autotools).

If you agree adding it to build_script is the right way to do that, I'll
do the same thing for the windows job.


regards

-- 
Tomas Vondra