On Mon, Mar 31, 2025 at 4:59 PM Bertrand Drouvot <bertranddrouvot...@gmail.com> wrote:
> Hi, Hi Bertrand, happy to see you back, thanks for review and here's v18 attached (an ideal fit for PG18 ;)) > On Mon, Mar 31, 2025 at 11:27:50AM +0200, Jakub Wartak wrote: > > On Thu, Mar 27, 2025 at 2:40 PM Andres Freund <and...@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()). Ok, let's have both just in case (this status is by default initialized to _UNKNOWN, so I hope you haven't had in mind using GetConfigOption() as this would need guc.h in port?) > === About v17-0002-This-extracts-code-from-contrib-pg_buffercache-s.patch [..] > I think pg_buffercache_numa_pages() should not be mentioned before it's > actually > implemented. Right, fixed. > === 1 > [..] > "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). Ouch, good catch. > === 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. Right, I've moved that into 003 where it belongs and now 002 has no single NUMA reference. I've thrown 0001+0002 alone onto CI and it passed too. -J.
v18-0002-This-extracts-code-from-contrib-pg_buffercache-s.patch
Description: Binary data
v18-0001-Add-optional-dependency-to-libnuma-Linux-only-fo.patch
Description: Binary data
v18-0004-Add-pg_shmem_numa_allocations-to-show-NUMA-memor.patch
Description: Binary data
v18-0003-Extend-pg_buffercache-with-new-view-pg_buffercac.patch
Description: Binary data
