Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.

On Thu, 9 May 2024 at 06:49, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> BTW, now that I've wrapped my head around what's happening here,
> I believe that -DREALLOCATE_BITMAPSETS is introducing a bug where
> there was none before. The changes that left-join removal makes
> won't cause any of these sets to go to empty, so the bms_del_member
> calls won't free the sets but just modify them in-place. And the
> same change will/should be made in every relevant relid set, so
> the fact that the sets may be shared isn't hurting anything.

FWIW, it just feels like we're willing to accept that the
bms_del_member() is not updating all copies of the set in this case as
that particular behaviour is ok for this particular case. I know
you're not proposing this, but I don't think that would warrant
relaxing REALLOCATE_BITMAPSETS to not reallocate Bitmapsets on
bms_del_member() and bms_del_members().

If all we have to do to make -DREALLOCATE_BITMAPSETS builds happy in
make check-world is to add a bms_copy inside the bms_del_member()
calls in remove_rel_from_query(), then I think it's a small price to
pay to allow us to maintain the additional coverage that
REALLOCATE_BITMAPSETS provides. That seems like a small price to pay
when the gains are removing an entire join.

> This conclusion also reinforces my previously-vague feeling that
> we should not consider making -DREALLOCATE_BITMAPSETS the default in
> debug builds, as was proposed upthread. It's really a fundamentally
> different behavior, and I strongly suspect that it can mask bugs as
> well as introduce them (by hiding sharing in cases that'd be less
> benign than this turns out to be). I'd rather not do development on
> top of bitmapset infrastructure that acts entirely different from
> production bitmapset infrastructure.

My primary interest in this feature is using it to catch bugs that
we're unlikely to ever hit in the regression tests. For example, the
planner works when there are <= 63 RTEs but falls over when there are
64 because some bms_add_member() must reallocate more memory to store
the 64th RTI in a Bitmapset. I'd like to have something to make it
more likely we'll find bugs like this before the release instead of
someone having a crash when they run some obscure query shape
containing > 63 RTEs 2 or 4 years after the release.

I'm happy Andrew added this to prion. Thanks for doing that.

David