From cfe30a72fa80528997357cb0780412736767e8c4 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
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 e0103c80431..b2cc9f07f56 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 cd4713f5e17..eafcd406dc0 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 2b9bd20bc6a..c9f34aa4e39 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
--
--
GitLab