On Sun, Nov 19, 2023 at 6:48 AM Alexander Korotkov <aekorot...@gmail.com> wrote: > > On Wed, Nov 15, 2023 at 5:07 PM Alexander Korotkov <aekorot...@gmail.com> > wrote: > > > > On Wed, Nov 15, 2023 at 8:02 AM Andres Freund <and...@anarazel.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
