David Rowley <dgrowle...@gmail.com> writes: > On Thu, 9 May 2024 at 06:49, Tom Lane <t...@sss.pgh.pa.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