public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: David Rowley <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: jian he <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
Date: Wed, 08 May 2024 18:16:19 -0400
Message-ID: <[email protected]> (raw)
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>
<[email protected]>
<[email protected]>
<CAApHDvoR9cXHb_+j7j8=zBty35B8z22SX51cZN7ZRf+QTPJ1fA@mail.gmail.com>
<[email protected]>
<CAApHDvrvcJd2DCAV0vDKmRxp5tvXq3=+RwhRns4OjoHQ1UbDEg@mail.gmail.com>
<[email protected]>
<CAApHDvqRgYq8yL3GUj-1z2xPv6cLy7D4tjxhhijWnsHLOSukLg@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAApHDvqZ8XwONN4bk-Eg38s_j5AY36aOhRn-vH=kx7n4Ve4XQQ@mail.gmail.com>
David Rowley <[email protected]> writes:
> On Thu, 9 May 2024 at 06:49, Tom Lane <[email protected]> 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
view thread (14+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox