Hi,
On Fri, Apr 04, 2025 at 09:25:57PM +0200, Tomas Vondra wrote:
> OK,
>
> here's v25 after going through the patches once more, fixing the issues
> mentioned by Bertrand, etc.
Thanks!
> I think 0001 and 0002 are fine,
Agree, I just have some cosmetic nits comments: please find them in
nit-bertrand-0002.txt attached.
> I have a
> couple minor questions about 0003.
>
> - I was wondering if maybe we should have some "global ID" of memory
> page, so that with large memory pages it's indicated the buffers are on
> the same memory page. Right now each buffer starts page_num from 0, but
> it should not be very hard to have a global counter. Opinions?
I think that's a good idea. We could then add a new column (say os_page_id) that
would help identify which buffers are sharing the same "physical" page.
> 0003
> ----
> - Minor formatting tweaks, comment improvements.
> - Isn't this comment a bit confusing / misleading?
>
> /* Get number of OS aligned pages */
>
> AFAICS the point is to adjust the allocated_size to be a multiple of
> os-page_size, to get "all" memory pages the segment uses. But that's not
> what I understand by "aligned page" (which is about there the page is
> expected to start).
Agree, what about?
"
Align the start of the allocated size to an OS page size boundary and then get
the total number of OS pages used by this segment"
"
> - There's a comment at the end which talks about "ignored segments".
> IMHO that type of information should be in the function comment,
> but I'm
> also not quite sure I understand what "output shared memory" is ...
I think that comes from the comments that are already in
pg_get_shmem_allocations().
I think that those are located here and worded that way to ease to understand
what is not in with pg_get_shmem_allocations_numa() if one look at both
functions. That said, I'm +1 to put this kind of comments in the function
comment.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
commit 17942e95d99d288658b6530e49795c28a93886d2
Author: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Sat Apr 5 08:56:03 2025 +0000
Nit
diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c
b/contrib/pg_buffercache/pg_buffercache_pages.c
index 65ade9d8135..0b96476c319 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -364,7 +364,7 @@ pg_buffercache_numa_pages(PG_FUNCTION_ARGS)
buffers_per_page, pages_per_buffer);
- /* initialize the multi-call context, load entries about
buffers */
+ /* Initialize the multi-call context, load entries about
buffers */
funcctx = SRF_FIRSTCALL_INIT();
@@ -412,7 +412,7 @@ pg_buffercache_numa_pages(PG_FUNCTION_ARGS)
MemoryContextSwitchTo(oldcontext);
- /* used to determine the NUMA node for all OS pages at once */
+ /* Used to determine the NUMA node for all OS pages at once */
os_page_ptrs = palloc0(sizeof(void *) * os_page_count);
os_page_status = palloc(sizeof(uint64) * os_page_count);
@@ -484,10 +484,10 @@ pg_buffercache_numa_pages(PG_FUNCTION_ARGS)
}
- /* we should get exactly the expected number of entrires */
+ /* We should get exactly the expected number of entrires */
Assert(idx == os_page_count);
- /* query NUMA status for all the pointers */
+ /* Query NUMA status for all the pointers */
if (pg_numa_query_pages(0, os_page_count, os_page_ptrs,
os_page_status) == -1)
elog(ERROR, "failed NUMA pages inquiry: %m");
@@ -500,7 +500,7 @@ pg_buffercache_numa_pages(PG_FUNCTION_ARGS)
fctx->record[i].numa_node = os_page_status[i];
}
- /* remember this backend touched the pages */
+ /* Remember this backend touched the pages */
firstNumaTouch = false;
}
