Re: Fix crash when non-creator being an iteration on shared radix tree

On Fri, Dec 20, 2024 at 9:55 PM John Naylor <johncnaylorls(at)gmail(dot)com> wrote:
>
> On Sat, Dec 21, 2024 at 2:17 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Fri, Dec 20, 2024 at 2:27 AM John Naylor <johncnaylorls(at)gmail(dot)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