Hi, On 2025-06-30 15:31:14 -0400, Burd, Greg wrote: > > On Jun 30, 2025, at 12:27 PM, Andres Freund <and...@anarazel.de> wrote: > > On 2025-06-05 14:32:10 -0400, Andres Freund wrote: > >> On 2025-06-05 12:47:52 -0400, Tom Lane wrote: > >>> Andres Freund <and...@anarazel.de> writes: > >>>> I think this is a big enough pitfall that it's, obviously assuming the > >>>> patch > >>>> has a sensible complexity, worth fixing this in 18. RMT, anyone, what do > >>>> you > >>>> think? > >>> > >>> Let's see the patch ... but yeah, I'd rather not ship 18 like this. > >> > >> I've attached a first draft. > >> > >> I can't make heads or tails of the ordering in configure.ac, so the > >> function > >> test is probably in the wrong place. > > > > Any comments on that patch? I'd hoped for some review comments... Unless > > I'll > > hear otherwise, I'll just do a bit more polish and push.. > > Thanks for doing this work! > > I just read through the v1 patch and it looks good. I have just a few small > nit-picky questions: > > + #if defined(HAVE_LIBURING_QUEUE_INIT_MEM) && defined(IORING_SETUP_NO_MMAP) > && 1 > > The '1' looks like cruft, or am I missing something?
It's for making it easy to test both paths when running on an kernel/liburing combo that's new enought o have support. > + /* FIXME: This should probably not stay at DEBUG1? */ > > Worth fixing before pushing? Yes. I was just not yet sure what it should be. I ended up concluding that it's probably fine to just keep it at DEBUG1... > Also, this returns 'Size' but in the function uses 'size_t' I assume that's > intentional? > > + static Size > + pgaio_uring_ring_shmem_size(void) > > The next, similar, function below this one returns 'size_t'. You're right - I wish we would just do a (slightly smarter) version of s/Size/size_t/... > Finally, and this may be me missing something everyone else knows is > convention. > > + * XXX: We allocate memory for all PgAioUringContext instances and, if > > Is there any reason to keep the 'XXX'? You ask yourself a question in that > comment, do you know the answer or was that a request to reviewers for > feedback? :) A bit of both :). I concluded that it's not worth having a separate segment, there's not enough memory here to matter... > I hope that is helpful. Yep! Greetings, Andres Freund
