From cfe30a72fa80528997357cb0780412736767e8c4 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 12 Aug 2015 21:18:45 -0400 Subject: [PATCH] Undo mistaken tightening in join_is_legal(). One of the changes I made in commit 8703059c6b55c427 turns out not to have been such a good idea: we still need the exception in join_is_legal() that allows a join if both inputs already overlap the RHS of the special join we're checking. Otherwise we can miss valid plans, and might indeed fail to find a plan at all, as in recent report from Andreas Seltenreich. That code was added way back in commit c17117649b9ae23d, but I failed to include a regression test case then; my bad. Put it back with a better explanation, and a test this time. The logic does end up a bit different than before though: I now believe it's appropriate to make this check first, thereby allowing such a case whether or not we'd consider the previous SJ(s) to commute with this one. (Presumably, we already decided they did; but it was confusing to have this consideration in the middle of the code that was handling the other case.) Back-patch to all active branches, like the previous patch. --- src/backend/optimizer/path/joinrels.c | 29 ++++++++++++++--- src/test/regress/expected/join.out | 46 +++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 19 +++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c index e0103c8043..b2cc9f07f5 100644 --- a/src/backend/optimizer/path/joinrels.c +++ b/src/backend/optimizer/path/joinrels.c @@ -470,11 +470,30 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, { /* * Otherwise, the proposed join overlaps the RHS but isn't a valid - * implementation of this SJ. It might still be a legal join, - * however, if we're allowed to associate it into the RHS of this - * SJ. That means this SJ must be a LEFT join (not SEMI or ANTI, - * and certainly not FULL) and the proposed join must not overlap - * the LHS. + * implementation of this SJ. But don't panic quite yet: the RHS + * violation might have occurred previously, in one or both input + * relations, in which case we must have previously decided that + * it was OK to commute some other SJ with this one. If we need + * to perform this join to finish building up the RHS, rejecting + * it could lead to not finding any plan at all. (This can occur + * because of the heuristics elsewhere in this file that postpone + * clauseless joins: we might not consider doing a clauseless join + * within the RHS until after we've performed other, validly + * commutable SJs with one or both sides of the clauseless join.) + * This consideration boils down to the rule that if both inputs + * overlap the RHS, we can allow the join --- they are either + * fully within the RHS, or represent previously-allowed joins to + * rels outside it. + */ + if (bms_overlap(rel1->relids, sjinfo->min_righthand) && + bms_overlap(rel2->relids, sjinfo->min_righthand)) + continue; /* assume valid previous violation of RHS */ + + /* + * The proposed join could still be legal, but only if we're + * allowed to associate it into the RHS of this SJ. That means + * this SJ must be a LEFT join (not SEMI or ANTI, and certainly + * not FULL) and the proposed join must not overlap the LHS. */ if (sjinfo->jointype != JOIN_LEFT || bms_overlap(joinrelids, sjinfo->min_lefthand)) diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index cd4713f5e1..eafcd406dc 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -3563,6 +3563,52 @@ select t1.* from hi de ho neighbor (2 rows) +explain (verbose, costs off) +select * from + text_tbl t1 + inner join int8_tbl i8 + on i8.q2 = 456 + right join text_tbl t2 + on t1.f1 = 'doh!' + left join int4_tbl i4 + on i8.q1 = i4.f1; + QUERY PLAN +-------------------------------------------------------- + Nested Loop Left Join + Output: t1.f1, i8.q1, i8.q2, t2.f1, i4.f1 + -> Seq Scan on public.text_tbl t2 + Output: t2.f1 + -> Materialize + Output: i8.q1, i8.q2, i4.f1, t1.f1 + -> Nested Loop + Output: i8.q1, i8.q2, i4.f1, t1.f1 + -> Nested Loop Left Join + Output: i8.q1, i8.q2, i4.f1 + Join Filter: (i8.q1 = i4.f1) + -> Seq Scan on public.int8_tbl i8 + Output: i8.q1, i8.q2 + Filter: (i8.q2 = 456) + -> Seq Scan on public.int4_tbl i4 + Output: i4.f1 + -> Seq Scan on public.text_tbl t1 + Output: t1.f1 + Filter: (t1.f1 = 'doh!'::text) +(19 rows) + +select * from + text_tbl t1 + inner join int8_tbl i8 + on i8.q2 = 456 + right join text_tbl t2 + on t1.f1 = 'doh!' + left join int4_tbl i4 + on i8.q1 = i4.f1; + f1 | q1 | q2 | f1 | f1 +------+-----+-----+-------------------+---- + doh! | 123 | 456 | doh! | + doh! | 123 | 456 | hi de ho neighbor | +(2 rows) + -- -- test ability to push constants through outer join clauses -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 2b9bd20bc6..c9f34aa4e3 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1108,6 +1108,25 @@ select t1.* from left join int4_tbl i4 on (i8.q2 = i4.f1); +explain (verbose, costs off) +select * from + text_tbl t1 + inner join int8_tbl i8 + on i8.q2 = 456 + right join text_tbl t2 + on t1.f1 = 'doh!' + left join int4_tbl i4 + on i8.q1 = i4.f1; + +select * from + text_tbl t1 + inner join int8_tbl i8 + on i8.q2 = 456 + right join text_tbl t2 + on t1.f1 = 'doh!' + left join int4_tbl i4 + on i8.q1 = i4.f1; + -- -- test ability to push constants through outer join clauses -- -- 2.40.0