On Fri, Dec 20, 2024 at 9:55 PM John Naylor <johncnaylorls@gmail.com> wrote:
>
> On Sat, Dec 21, 2024 at 2:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Dec 20, 2024 at 2:27 AM John Naylor <johncnaylorls@gmail.com> wrote:
>
> > > v3-0001 allocates the iter data in the caller's context. It's a small
> > > patch, but still a core behavior change so proposed for master-only. I
> > > believe your v1 is still the least invasive fix for PG17.
> >
> > I agree to use v1 for v17.
>
> Okay, did you want to commit that separately, or together with my 0001
> on master? Either way, I've put a bit more effort into the commit
> message in v4 and adjusted the comment slightly.
I think we can commit them separately. It's a pure bug fix for back
branches while for HEAD it removes the bug by an improvement.
>
> > > It's trivial after 0001-02: 0003 removes makes one test use slab as
> > > the passed context (only 32-bit systems would actually use it
> > > currently).
> >
> > These changes make sense to me. Here are a few comments:
> >
> > RT_RADIX_TREE has 'leaf_context' but it seems that we use it only for
> > local memory. Do we want to declare only in the !RT_SHMEM case?
>
> That's already the case, if I understand your statement correctly.
True. Sorry, I misunderstood something.
>
> > ---
> > /*
> > * Similar to TidStoreCreateLocal() but create a shared TidStore on a
> > * DSA area. The TID storage will live in the DSA area, and the memory
> > * context rt_context will have only meta data of the radix tree.
> > *
> > * The returned object is allocated in backend-local memory.
> > */
> >
> > We need to update the comment about rt_context too since we no longer
> > use rt_context for shared tidstore.
>
> Fixed.
The three patches look good to me.
> BTW, it seems TidStore.context is unused?
Indeed. We can remove it.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
