From: | Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com> |
---|---|
To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Draft for basic NUMA observability |
Date: | 2025-03-17 07:28:46 |
Message-ID: | CAKZiRmxrnyGN9TAc_pBL5HCEgTG+NpK01DzaheTXB07zb4_80w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Mar 14, 2025 at 1:08 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> On Fri, Mar 14, 2025 at 11:05:28AM +0100, Jakub Wartak wrote:
> > On Thu, Mar 13, 2025 at 3:15 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > Thank you very much for the review! I'm answering to both reviews in
> > one go and the results is attached v12, seems it all should be solved
> > now:
>
> Thanks for v12!
>
> I'll review 0001 and 0003 later, but want to share what I've done for 0002.
>
> I did prepare a patch file (attached as .txt to not disturb the cfbot) to apply
> on top of v11 0002 (I just rebased it a bit so that it now applies on top of
> v12 0002).
Hey Bertrand,
all LGTM (good ideas), so here's v13 attached with applied all of that
(rebased, tested). BTW: I'm sending to make cfbot as it still tried to
apply that .patch (on my side it .patch, not .txt)
> === 9
>
> -static bool firstUseInBackend = true;
> +static bool firstNumaTouch = true;
>
> Looks better to me but still not 100% convinced by the name.
IMHO, Yes, it looks much better.
> === 10
>
> static BufferCachePagesContext *
> -pg_buffercache_init_entries(FuncCallContext *funcctx, PG_FUNCTION_ARGS)
> +pg_buffercache_init_entries(FuncCallContext *funcctx, FunctionCallInfo fcinfo)
>
> as PG_FUNCTION_ARGS is usually used for fmgr-compatible function and there is
> a lof of examples in the code that make use of "FunctionCallInfo" for non
> fmgr-compatible function.
Cool, thanks.
> and also:
>
> === 11
>
> I don't like the fact that we iterate 2 times over NBuffers in
> pg_buffercache_numa_pages().
>
> But I'm having having hard time finding a better approach given the fact that
> pg_numa_query_pages() needs all the pointers "prepared" before it can be called...
>
> Those 2 loops are probably the best approach, unless someone has a better idea.
IMHO, it doesn't hurt and I've also not been able to think of any better idea.
> === 12
>
> Upthread you asked "Can you please take a look again on this, is this up to the
> project standards?"
>
> Was the question about using pg_buffercache_numa_prepare_ptrs() as an inlined wrapper?
Yes, this was for an earlier doubt regarding question "19" about
reviewing the code after removal of `query_numa` variable. This is the
same code for 2 loops, IMHO it is good now.
> What do you think? The comments, doc and code changes are just proposals and are
> fully open to discussion.
They are great, thank You!
-J.
Attachment | Content-Type | Size |
---|---|---|
v13-0001-Add-optional-dependency-to-libnuma-Linux-only-fo.patch | application/x-patch | 18.8 KB |
v13-0003-Add-pg_shmem_numa_allocations-to-show-NUMA-zones.patch | application/x-patch | 15.9 KB |
v13-0002-Extend-pg_buffercache-with-new-view-pg_buffercac.patch | application/x-patch | 28.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bykov Ivan | 2025-03-17 07:33:42 | RE: Query ID Calculation Fix for DISTINCT / ORDER BY and LIMIT / OFFSET |
Previous Message | Tom Lane | 2025-03-17 07:09:25 | Re: 64 bit numbers vs format strings |