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: <815049.1715192644@sss.pgh.pa.us>
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>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 08 May 2024 14:24:04 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <818431.1715194157.1@sss.pgh.pa.us>
Date: Wed, 08 May 2024 14:49:17 -0400
Message-ID: <818432.1715194157@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/818432.1715194157%40sss.pgh.pa.us>
Precedence: bulk

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.

This is, of course, pretty fragile and I'm totally on board with
making it safer.  But there's no live bug in released branches,
and not in HEAD either unless you add -DREALLOCATE_BITMAPSETS.
That accounts for the lack of related field reports, and it means
we don't really need to back-patch anything.

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.

			regards, tom lane