Re: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'

On Sun, Nov 19, 2023 at 6:48 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
>
> On Wed, Nov 15, 2023 at 5:07 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> >
> > On Wed, Nov 15, 2023 at 8:02 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > The kinda because there are callers to bms_(add|del)_members() that pass the
> > > same bms as a and b, which only works if the reallocation happens "late".
> >
> > +1,
> > Neat idea. I'm willing to work on this. Will propose the patch soon.
>
>
> It's here. New REALLOCATE_BITMAPSETS forces bitmapset reallocation on
> each modification. I also find it useful to add assert to all
> bitmapset functions on argument NodeTag. This allows you to find
> access to hanging pointers earlier.

Creating separate patches for REALLOCATE_BITMAPSETs and
Assert(ISA(Bitmapset)) will be easier to review. We will be able to
check whether all the places that require either of the fixes have
been indeed fixed and correctly. I kept switching back and forth.

>
> 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.

I think this will increase memory consumption when planning queries
with partitioned tables (100s or 1000s of partitions). Have you tried
measuring the impact?

We should take hit on memory consumption when there is correctness
involved but not all these cases look correctness problems. For
example. RelOptInfo::left_relids or SpecialJoinInfo::syn_lefthand may
not get modified after they are set. But just because
RelOptInfo::relids of a lower relation was assigned somewhere which
got modified, these two get modified. bms_copy() in
make_specialjoininfo may not be necessary. I haven't tried that myself
so I may be wrong.

What might be useful is to mark a bitmap as "final" once it's know
that it can not change. e.g. RelOptInfo->relids once set never
changes. Each operation that modifies a Bitmapset throws an
error/Asserts if it's marked as "final", thus catching the places
where we expect a Bitmapset being modified when not intended. This
will catch shared bitmapsets as well. We could apply bms_copy in only
those cases then.

--
Best Wishes,
Ashutosh Bapat