Fix over-optimistic updating of info about commutable outer joins.

tglsfdc · tglsfdc · commit b2d0e13a0a4c · 2023-02-05T14:25:10.000-05:00
make_outerjoininfo was set up to update SpecialJoinInfo's commute_below, commute_above_l, commute_above_r fields as soon as it found a pair of outer joins that look like they can commute. However, this decision could be negated later in the same loop due to finding an intermediate outer join that prevents commutation. That left us with commute_xxx fields that were contradictory to the join order restrictions expressed in min_lefthand/min_righthand. The latter fields would keep us from actually choosing a bad join order; but the inconsistent commute_xxx fields could bollix details such as the varnullingrels values created for intermediate join relation targetlists, ending in an assertion failure in setrefs.c. To fix, wait till the end of make_outerjoininfo where we have accurate values for min_lefthand/min_righthand, and then insert only relids not present in those sets into the commute_xxx fields. Per SQLSmith testing by Robins Tharakan. Note that while Robins bisected the failure to commit b448f1c, it's really the fault of 2489d76. The outerjoin_delayed logic removed in the later commit was keeping us from deciding that troublesome join pairs commute, at least in the specific example seen here. Discussion: https://postgr.es/m/CAEP4nAyAORgE8K_RHSmvWbE9UaChhjbEL1RrDU3neePwwRUB=A@mail.gmail.com
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
@@ -1358,6 +1358,8 @@ make_outerjoininfo(PlannerInfo *root,
 	Relids		strict_relids;
 	Relids		min_lefthand;
 	Relids		min_righthand;
+	Relids		commute_below_l;
+	Relids		commute_below_r;
 	ListCell   *l;
 
 	/*
@@ -1445,7 +1447,14 @@ make_outerjoininfo(PlannerInfo *root,
 
 	/*
 	 * Now check previous outer joins for ordering restrictions.
+	 *
+	 * commute_below_l and commute_below_r accumulate the relids of lower
+	 * outer joins that we think this one can commute with.  These decisions
+	 * are just tentative within this loop, since we might find an
+	 * intermediate outer join that prevents commutation.  Surviving relids
+	 * will get merged into the SpecialJoinInfo structs afterwards.
 	 */
+	commute_below_l = commute_below_r = NULL;
 	foreach(l, root->join_info_list)
 	{
 		SpecialJoinInfo *otherinfo = (SpecialJoinInfo *) lfirst(l);
@@ -1458,16 +1467,16 @@ make_outerjoininfo(PlannerInfo *root,
 		 */
 		if (otherinfo->jointype == JOIN_FULL)
 		{
+			Assert(otherinfo->ojrelid != 0);
 			if (bms_overlap(left_rels, otherinfo->syn_lefthand) ||
 				bms_overlap(left_rels, otherinfo->syn_righthand))
 			{
 				min_lefthand = bms_add_members(min_lefthand,
 											   otherinfo->syn_lefthand);
 				min_lefthand = bms_add_members(min_lefthand,
 											   otherinfo->syn_righthand);
-				if (otherinfo->ojrelid != 0)
-					min_lefthand = bms_add_member(min_lefthand,
-												  otherinfo->ojrelid);
+				min_lefthand = bms_add_member(min_lefthand,
+											  otherinfo->ojrelid);
 			}
 			if (bms_overlap(right_rels, otherinfo->syn_lefthand) ||
 				bms_overlap(right_rels, otherinfo->syn_righthand))
@@ -1476,9 +1485,8 @@ make_outerjoininfo(PlannerInfo *root,
 												otherinfo->syn_lefthand);
 				min_righthand = bms_add_members(min_righthand,
 												otherinfo->syn_righthand);
-				if (otherinfo->ojrelid != 0)
-					min_righthand = bms_add_member(min_righthand,
-												   otherinfo->ojrelid);
+				min_righthand = bms_add_member(min_righthand,
+											   otherinfo->ojrelid);
 			}
 			/* Needn't do anything else with the full join */
 			continue;
@@ -1536,11 +1544,9 @@ make_outerjoininfo(PlannerInfo *root,
 			{
 				/* Identity 3 applies, so remove the ordering restriction */
 				min_lefthand = bms_del_member(min_lefthand, otherinfo->ojrelid);
-				/* Add commutability markers to both SpecialJoinInfos */
-				otherinfo->commute_above_l =
-					bms_add_member(otherinfo->commute_above_l, ojrelid);
-				sjinfo->commute_below =
-					bms_add_member(sjinfo->commute_below, otherinfo->ojrelid);
+				/* Record the (still tentative) commutability relationship */
+				commute_below_l =
+					bms_add_member(commute_below_l, otherinfo->ojrelid);
 			}
 		}
 
@@ -1589,11 +1595,9 @@ make_outerjoininfo(PlannerInfo *root,
 				/* Identity 3 applies, so remove the ordering restriction */
 				min_righthand = bms_del_member(min_righthand,
 											   otherinfo->ojrelid);
-				/* Add commutability markers to both SpecialJoinInfos */
-				otherinfo->commute_above_r =
-					bms_add_member(otherinfo->commute_above_r, ojrelid);
-				sjinfo->commute_below =
-					bms_add_member(sjinfo->commute_below, otherinfo->ojrelid);
+				/* Record the (still tentative) commutability relationship */
+				commute_below_r =
+					bms_add_member(commute_below_r, otherinfo->ojrelid);
 			}
 		}
 	}
@@ -1639,6 +1643,44 @@ make_outerjoininfo(PlannerInfo *root,
 	sjinfo->min_lefthand = min_lefthand;
 	sjinfo->min_righthand = min_righthand;
 
+	/*
+	 * Now that we've identified the correct min_lefthand and min_righthand,
+	 * any commute_below_l or commute_below_r relids that have not gotten
+	 * added back into those sets (due to intervening outer joins) are indeed
+	 * commutable with this one.  Update the derived data in the
+	 * SpecialJoinInfos.
+	 */
+	if (commute_below_l || commute_below_r)
+	{
+		Relids		commute_below;
+
+		/*
+		 * Delete any subsequently-added-back relids (this is easier than
+		 * maintaining commute_below_l/r precisely through all the above).
+		 */
+		commute_below_l = bms_del_members(commute_below_l, min_lefthand);
+		commute_below_r = bms_del_members(commute_below_r, min_righthand);
+
+		/* Anything left? */
+		commute_below = bms_union(commute_below_l, commute_below_r);
+		if (!bms_is_empty(commute_below))
+		{
+			/* Yup, so we must update the data structures */
+			sjinfo->commute_below = commute_below;
+			foreach(l, root->join_info_list)
+			{
+				SpecialJoinInfo *otherinfo = (SpecialJoinInfo *) lfirst(l);
+
+				if (bms_is_member(otherinfo->ojrelid, commute_below_l))
+					otherinfo->commute_above_l =
+						bms_add_member(otherinfo->commute_above_l, ojrelid);
+				else if (bms_is_member(otherinfo->ojrelid, commute_below_r))
+					otherinfo->commute_above_r =
+						bms_add_member(otherinfo->commute_above_r, ojrelid);
+			}
+		}
+	}
+
 	return sjinfo;
 }
 
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
@@ -4469,6 +4469,36 @@ left join
          One-Time Filter: false
 (5 rows)
 
+-- check handling of apparently-commutable outer joins with non-commutable
+-- joins between them
+explain (costs off)
+select 1 from
+  int4_tbl i4
+  left join int8_tbl i8 on i4.f1 is not null
+  left join (select 1 as a) ss1 on null
+  join int4_tbl i42 on ss1.a is null or i8.q1 <> i8.q2
+  right join (select 2 as b) ss2
+  on ss2.b < i4.f1;
+                        QUERY PLAN                         
+-----------------------------------------------------------
+ Nested Loop Left Join
+   ->  Result
+   ->  Nested Loop
+         ->  Nested Loop Left Join
+               Join Filter: NULL::boolean
+               Filter: (((1) IS NULL) OR (i8.q1 <> i8.q2))
+               ->  Nested Loop Left Join
+                     Join Filter: (i4.f1 IS NOT NULL)
+                     ->  Seq Scan on int4_tbl i4
+                           Filter: (2 < f1)
+                     ->  Materialize
+                           ->  Seq Scan on int8_tbl i8
+               ->  Result
+                     One-Time Filter: false
+         ->  Materialize
+               ->  Seq Scan on int4_tbl i42
+(16 rows)
+
 --
 -- test for appropriate join order in the presence of lateral references
 --
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
@@ -1545,6 +1545,17 @@ left join
   where c.relkind = 'r'
 ) ss2 on false;
 
+-- check handling of apparently-commutable outer joins with non-commutable
+-- joins between them
+explain (costs off)
+select 1 from
+  int4_tbl i4
+  left join int8_tbl i8 on i4.f1 is not null
+  left join (select 1 as a) ss1 on null
+  join int4_tbl i42 on ss1.a is null or i8.q1 <> i8.q2
+  right join (select 2 as b) ss2
+  on ss2.b < i4.f1;
+
 --
 -- test for appropriate join order in the presence of lateral references
 --
