From 8703059c6b55c427100e00a09f66534b6ccbfaa1 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 6 Aug 2015 15:35:27 -0400 Subject: Further fixes for degenerate outer join clauses. Further testing revealed that commit f69b4b9495269cc4 was still a few bricks shy of a load: minor tweaking of the previous test cases resulted in the same wrong-outer-join-order problem coming back. After study I concluded that my previous changes in make_outerjoininfo() were just accidentally masking the problem, and should be reverted in favor of forcing syntactic join order whenever an upper outer join's predicate doesn't mention a lower outer join's LHS. This still allows the chained-outer-joins style that is the normally optimizable case. I also tightened things up some more in join_is_legal(). It seems to me on review that what's really happening in the exception case where we ignore a mismatched special join is that we're allowing the proposed join to associate into the RHS of the outer join we're comparing it to. As such, we should *always* insist that the proposed join be a left join, which eliminates a bunch of rather dubious argumentation. The case where we weren't enforcing that was the one that was already known buggy anyway (it had a violatable Assert before the aforesaid commit) so it hardly deserves a lot of deference. Back-patch to all active branches, like the previous patch. The added regression test case failed in all branches back to 9.1, and I think it's only an unrelated change in costing calculations that kept 9.0 from choosing a broken plan. --- src/backend/optimizer/plan/initsplan.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) (limited to 'src/backend/optimizer/plan/initsplan.c') diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 40a867c2607..2f4e8181eb5 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -1128,17 +1128,6 @@ make_outerjoininfo(PlannerInfo *root, min_righthand = bms_int_members(bms_union(clause_relids, inner_join_rels), right_rels); - /* - * If we have a degenerate join clause that doesn't mention any RHS rels, - * force the min RHS to be the syntactic RHS; otherwise we can end up - * making serious errors, like putting the LHS on the wrong side of an - * outer join. It seems to be safe to not do this when we have a - * contribution from inner_join_rels, though; that's enough to pin the SJ - * to occur at a reasonable place in the tree. - */ - if (bms_is_empty(min_righthand)) - min_righthand = bms_copy(right_rels); - /* * Now check previous outer joins for ordering restrictions. */ @@ -1181,9 +1170,15 @@ make_outerjoininfo(PlannerInfo *root, * For a lower OJ in our RHS, if our join condition does not use the * lower join's RHS and the lower OJ's join condition is strict, we * can interchange the ordering of the two OJs; otherwise we must add - * the lower OJ's full syntactic relset to min_righthand. Also, we - * must preserve ordering anyway if either the current join or the - * lower OJ is either a semijoin or an antijoin. + * the lower OJ's full syntactic relset to min_righthand. + * + * Also, if our join condition does not use the lower join's LHS + * either, force the ordering to be preserved. Otherwise we can end + * up with SpecialJoinInfos with identical min_righthands, which can + * confuse join_is_legal (see discussion in backend/optimizer/README). + * + * Also, we must preserve ordering anyway if either the current join + * or the lower OJ is either a semijoin or an antijoin. * * Here, we have to consider that "our join condition" includes any * clauses that syntactically appeared above the lower OJ and below @@ -1199,6 +1194,7 @@ make_outerjoininfo(PlannerInfo *root, if (bms_overlap(right_rels, otherinfo->syn_righthand)) { if (bms_overlap(clause_relids, otherinfo->syn_righthand) || + !bms_overlap(clause_relids, otherinfo->min_lefthand) || jointype == JOIN_SEMI || jointype == JOIN_ANTI || otherinfo->jointype == JOIN_SEMI || @@ -1238,10 +1234,12 @@ make_outerjoininfo(PlannerInfo *root, * If we found nothing to put in min_lefthand, punt and make it the full * LHS, to avoid having an empty min_lefthand which will confuse later * processing. (We don't try to be smart about such cases, just correct.) - * We already forced min_righthand nonempty, so nothing to do for that. + * Likewise for min_righthand. */ if (bms_is_empty(min_lefthand)) min_lefthand = bms_copy(left_rels); + if (bms_is_empty(min_righthand)) + min_righthand = bms_copy(right_rels); /* Now they'd better be nonempty */ Assert(!bms_is_empty(min_lefthand)); -- cgit v1.2.3