On Mon, Mar 31, 2025 at 4:59 PM Bertrand Drouvot
<bertranddrouvot.pg@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 <andres@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.
