From b2d0e13a0a4c31167d01e9871f907060c80b8fae Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 5 Feb 2023 14:25:10 -0500 Subject: [PATCH] Fix over-optimistic updating of info about commutable outer joins. 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 b448f1c8d, it's really the fault of 2489d76c4. 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 --- src/backend/optimizer/plan/initsplan.c | 74 ++++++++++++++++++++------ src/test/regress/expected/join.out | 30 +++++++++++ src/test/regress/sql/join.sql | 11 ++++ 3 files changed, 99 insertions(+), 16 deletions(-) diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index c301e6dffc..2d9aa85e59 100644 --- 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,6 +1467,7 @@ 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)) { @@ -1465,9 +1475,8 @@ make_outerjoininfo(PlannerInfo *root, 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 index 9762952efd..037c7d0d56 100644 --- 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 index 3ef2996040..1f2b7f62f0 100644 --- 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 --