Re: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)' - Mailing list pgsql-hackers

On Fri, Nov 24, 2023 at 3:54 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Thu, Nov 23, 2023 at 4:33 AM Richard Guo <guofenglinux@gmail.com> wrote:
> >
> > On Sun, Nov 19, 2023 at 9:17 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >>
> >> It's here.  New REALLOCATE_BITMAPSETS forces bitmapset reallocation on
> >> each modification.
> >
> >
> > +1 to the idea of introducing a reallocation mode to Bitmapset.
> >
> >>
> >> I had the feeling of falling into a rabbit hole while debugging all
> >> the cases of failure with this new option.  With the second patch
> >> regressions tests pass.
> >
> >
> > It seems to me that we have always had situations where we share the
> > same pointer to a Bitmapset structure across different places.  I do not
> > think this is a problem as long as we do not modify the Bitmapsets in a
> > way that requires reallocation or impact the locations sharing the same
> > pointer.
> >
> > So I'm wondering, instead of attempting to avoid sharing pointer to
> > Bitmapset in all locations that have problems, can we simply bms_copy
> > the original Bitmapset within replace_relid() before making any
> > modifications, as I proposed previously?  Of course, as Andres pointed
> > out, we need to do so also for the "Delete relid without substitution"
> > path.  Please see the attached.
>
>
> Yes, this makes sense.  Thank you for the patch.  My initial point was
> that replace_relid() should either do in-place in all cases or make a
> copy in all cases.  Now I see that it should make a copy in all cases.
> Note, that without making a copy in delete case, regression tests fail
> with REALLOCATE_BITMAPSETS on.
>
> Please, find the revised patchset.  As Ashutosh Bapat asked, asserts
> are split into separate patch.

Any objections to pushing this?

------
Regards,
Alexander Korotkov