From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Rowley <dgrowleyml@gmail.com>
cc: Andrew Dunstan <andrew@dunslane.net>, Richard Guo <guofenglinux@gmail.com>,
        jian he <jian.universality@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
In-reply-to: <CAApHDvqZ8XwONN4bk-Eg38s_j5AY36aOhRn-vH=kx7n4Ve4XQQ@mail.gmail.com>
References: <CACJufxFVQmr4=JWHAOSLuKA5Zy9H26nY6tVrRFBOekHoALyCkQ@mail.gmail.com> <CAApHDvpBMSGhXVeFdw_qmjPqU5yHvJJ7k0z7kD069EC4di2Pag@mail.gmail.com> <CAMbWs4-iYKxrMs8kj+49gVA4X1_=PYAaw5HL_3g_yVXpAHzYww@mail.gmail.com> <1e6ca7b3-98e6-46b2-9982-97f92c7523d8@dunslane.net> <76910.1715106055@sss.pgh.pa.us> <CAApHDvoR9cXHb_+j7j8=zBty35B8z22SX51cZN7ZRf+QTPJ1fA@mail.gmail.com> <276516.1715121616@sss.pgh.pa.us> <CAApHDvrvcJd2DCAV0vDKmRxp5tvXq3=+RwhRns4OjoHQ1UbDEg@mail.gmail.com> <279607.1715122556@sss.pgh.pa.us> <CAApHDvqRgYq8yL3GUj-1z2xPv6cLy7D4tjxhhijWnsHLOSukLg@mail.gmail.com> <282081.1715123889@sss.pgh.pa.us> <815049.1715192644@sss.pgh.pa.us> <818432.1715194157@sss.pgh.pa.us> <CAApHDvqZ8XwONN4bk-Eg38s_j5AY36aOhRn-vH=kx7n4Ve4XQQ@mail.gmail.com>
Comments: In-reply-to David Rowley <dgrowleyml@gmail.com>
	message dated "Thu, 09 May 2024 10:02:06 +1200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <933605.1715206579.1@sss.pgh.pa.us>
Date: Wed, 08 May 2024 18:16:19 -0400
Message-ID: <933606.1715206579@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/933606.1715206579%40sss.pgh.pa.us>
Precedence: bulk

David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 9 May 2024 at 06:49, Tom Lane <tgl@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