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

David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> 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,

No, I'm not. I was just trying to explain how come there's not
a visible bug. I quite agree that this is too fragile to leave
as-is going forward. (One thing I'm wondering about is whether
we should back-patch despite the lack of visible bug, just in
case some future back-patch relies on the safer behavior.)

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

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

Again, I think -DREALLOCATE_BITMAPSETS adds a valuable testing weapon.
But if we make that the default in debug builds, then we'll get next
door to zero testing of the behavior without it, and that seems like
a really bad idea given how different the behavior is.

(Speaking of which, I wonder how many buildfarm members build without
--enable-cassert. The answer could well be "zero", and that's likely
not good.)

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

+1

regards, tom lane