Hi,
On 2025-06-30 15:31:14 -0400, Burd, Greg wrote:
> > On Jun 30, 2025, at 12:27 PM, Andres Freund <andres@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 <andres@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
