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: <282081.1715123889@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>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 07 May 2024 19:18:09 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <815014.1715192616.0@sss.pgh.pa.us>
Date: Wed, 08 May 2024 14:24:04 -0400
Message-ID: <815049.1715192644@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/815049.1715192644%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <815014.1715192616.1@sss.pgh.pa.us>

I traced down the other failure I was seeing in check-world, and
found that it came from deconstruct_distribute doing this:

        distribute_quals_to_rels(root, my_quals,
                                 jtitem,
                                 sjinfo,
                                 root->qual_security_level,
                                 jtitem->qualscope,
                                 ojscope, jtitem->nonnullable_rels,
                                 NULL,    /* incompatible_relids */
                                 true,    /* allow_equivalence */
                                 false, false,    /* not clones */
                                 postponed_oj_qual_list);

where jtitem->nonnullable_rels is the same as the jtitem's left_rels,
which ends up as the syn_lefthand of the join's SpecialJoinInfo, and
then when remove_rel_from_query tries to adjust the syn_lefthand it
breaks the outer_relids of whatever RestrictInfos got made here.

I was able to fix that by not letting jtitem->nonnullable_rels be
the same as left_rels.  The attached alternative_1.patch does pass
check-world.  But I find it mighty unprincipled: the JoinTreeItem
data structures are full of shared relid sets, so why is this
particular sharing not OK?  I still don't have any confidence that
there aren't more problems.

Along about here I started to wonder how come we are only seeing
SpecialJoinInfo-vs-RestrictInfo sharing as a problem, when surely
there is plenty of cross-RestrictInfo sharing going on as well.
(The above call is perfectly capable of making a lot of RestrictInfos,
all with the same outer_relids.)  That thought led me to look at
remove_rel_from_restrictinfo, and darn if I didn't find this:

    /*
     * The clause_relids probably aren't shared with anything else, but let's
     * copy them just to be sure.
     */
    rinfo->clause_relids = bms_copy(rinfo->clause_relids);
    ...
    /* Likewise for required_relids */
    rinfo->required_relids = bms_copy(rinfo->required_relids);

So the reason we don't see cross-RestrictInfo breakage is that
analyzejoins.c is careful not to modify the original relid sets
when modifying a RestrictInfo.  (This comment is clearly wrong.)

And that leads to the thought that we can fix our current sharing
problems by similarly avoiding overwriting the original sets
in remove_rel_from_query.  The attached alternative-2.patch
attacks it that way, and also passes check-world.

I like alternative-2.patch a lot better, not least because it
only adds cycles when join removal actually fires.  Basically
this is putting the onus on the data structure modifier to
cope with shared bitmapsets, rather than trying to say that
sharing is disallowed.

Thoughts?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="alternative-1.patch"; charset="us-ascii"
Content-ID: <815014.1715192616.2@sss.pgh.pa.us>
Content-Description: alternative-1.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimize=
r/plan/initsplan.c
index e2c68fe6f9..4198e9c83a 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -979,7 +979,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
 													right_item->inner_join_rels);
 				jtitem->left_rels =3D left_item->qualscope;
 				jtitem->right_rels =3D right_item->qualscope;
-				jtitem->nonnullable_rels =3D left_item->qualscope;
+				jtitem->nonnullable_rels =3D bms_copy(left_item->qualscope);
 				break;
 			case JOIN_SEMI:
 				/* This node belongs to parent_domain, as do its children */
@@ -1053,7 +1053,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
 				jtitem->left_rels =3D left_item->qualscope;
 				jtitem->right_rels =3D right_item->qualscope;
 				/* each side is both outer and inner */
-				jtitem->nonnullable_rels =3D jtitem->qualscope;
+				jtitem->nonnullable_rels =3D bms_copy(jtitem->qualscope);
 				break;
 			default:
 				/* JOIN_RIGHT was eliminated during reduce_outer_joins() */
@@ -1888,7 +1888,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
 	qualscope =3D bms_union(sjinfo->syn_lefthand, sjinfo->syn_righthand);
 	qualscope =3D bms_add_member(qualscope, sjinfo->ojrelid);
 	ojscope =3D bms_union(sjinfo->min_lefthand, sjinfo->min_righthand);
-	nonnullable_rels =3D sjinfo->syn_lefthand;
+	nonnullable_rels =3D bms_copy(sjinfo->syn_lefthand);
 =

 	/*
 	 * If this join can commute with any other ones per outer-join identity =
3,

------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="alternative-2.patch"; charset="us-ascii"
Content-ID: <815014.1715192616.3@sss.pgh.pa.us>
Content-Description: alternative-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optim=
izer/plan/analyzejoins.c
index aa72592567..1c9acf864c 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -390,6 +390,17 @@ remove_rel_from_query(PlannerInfo *root, int relid, S=
pecialJoinInfo *sjinfo)
 	{
 		SpecialJoinInfo *sjinf =3D (SpecialJoinInfo *) lfirst(l);
 =

+		/*
+		 * initsplan.c is fairly cavalier about allowing SpecialJoinInfos'
+		 * lefthand/righthand relid sets to be shared with other data
+		 * structures.  Ensure that we don't modify the original relid sets.
+		 * (The commute_xxx sets are always per-SpecialJoinInfo though.)
+		 */
+		sjinf->min_lefthand =3D bms_copy(sjinf->min_lefthand);
+		sjinf->min_righthand =3D bms_copy(sjinf->min_righthand);
+		sjinf->syn_lefthand =3D bms_copy(sjinf->syn_lefthand);
+		sjinf->syn_righthand =3D bms_copy(sjinf->syn_righthand);
+		/* Now remove relid and ojrelid bits from the sets: */
 		sjinf->min_lefthand =3D bms_del_member(sjinf->min_lefthand, relid);
 		sjinf->min_righthand =3D bms_del_member(sjinf->min_righthand, relid);
 		sjinf->syn_lefthand =3D bms_del_member(sjinf->syn_lefthand, relid);
@@ -551,8 +562,10 @@ static void
 remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid)
 {
 	/*
-	 * The clause_relids probably aren't shared with anything else, but let'=
s
-	 * copy them just to be sure.
+	 * initsplan.c is fairly cavalier about allowing RestrictInfos to share
+	 * relid sets with other RestrictInfos (and SpecialJoinInfos for that
+	 * matter).  To avoid breaking things, make sure this RestrictInfo has i=
ts
+	 * own clause_relids set before we modify it.
 	 */
 	rinfo->clause_relids =3D bms_copy(rinfo->clause_relids);
 	rinfo->clause_relids =3D bms_del_member(rinfo->clause_relids, relid);

------- =_aaaaaaaaaa0--