Skip to content

Commit 8cd3a7a

Browse files
tglsfdctglsfdc
committed
Undo mistaken tightening in join_is_legal().
One of the changes I made in commit 8703059 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 c171176, 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.
1 parent 157d406 commit 8cd3a7a

File tree

3 files changed

+89
-5
lines changed

3 files changed

+89
-5
lines changed

‎src/backend/optimizer/path/joinrels.c

+24-5
Original file line numberDiff line numberDiff line change
@@ -470,11 +470,30 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
470470
{
471471
/*
472472
* Otherwise, the proposed join overlaps the RHS but isn't a valid
473-
* implementation of this SJ. It might still be a legal join,
474-
* however, if we're allowed to associate it into the RHS of this
475-
* SJ. That means this SJ must be a LEFT join (not SEMI or ANTI,
476-
* and certainly not FULL) and the proposed join must not overlap
477-
* the LHS.
473+
* implementation of this SJ. But don't panic quite yet: the RHS
474+
* violation might have occurred previously, in one or both input
475+
* relations, in which case we must have previously decided that
476+
* it was OK to commute some other SJ with this one. If we need
477+
* to perform this join to finish building up the RHS, rejecting
478+
* it could lead to not finding any plan at all. (This can occur
479+
* because of the heuristics elsewhere in this file that postpone
480+
* clauseless joins: we might not consider doing a clauseless join
481+
* within the RHS until after we've performed other, validly
482+
* commutable SJs with one or both sides of the clauseless join.)
483+
* This consideration boils down to the rule that if both inputs
484+
* overlap the RHS, we can allow the join --- they are either
485+
* fully within the RHS, or represent previously-allowed joins to
486+
* rels outside it.
487+
*/
488+
if (bms_overlap(rel1->relids, sjinfo->min_righthand) &&
489+
bms_overlap(rel2->relids, sjinfo->min_righthand))
490+
continue; /* assume valid previous violation of RHS */
491+
492+
/*
493+
* The proposed join could still be legal, but only if we're
494+
* allowed to associate it into the RHS of this SJ. That means
495+
* this SJ must be a LEFT join (not SEMI or ANTI, and certainly
496+
* not FULL) and the proposed join must not overlap the LHS.
478497
*/
479498
if (sjinfo->jointype != JOIN_LEFT ||
480499
bms_overlap(joinrelids, sjinfo->min_lefthand))

‎src/test/regress/expected/join.out

+46
Original file line numberDiff line numberDiff line change
@@ -3523,6 +3523,52 @@ select t1.* from
35233523
hi de ho neighbor
35243524
(2 rows)
35253525

3526+
explain (verbose, costs off)
3527+
select * from
3528+
text_tbl t1
3529+
inner join int8_tbl i8
3530+
on i8.q2 = 456
3531+
right join text_tbl t2
3532+
on t1.f1 = 'doh!'
3533+
left join int4_tbl i4
3534+
on i8.q1 = i4.f1;
3535+
QUERY PLAN
3536+
--------------------------------------------------------
3537+
Nested Loop Left Join
3538+
Output: t1.f1, i8.q1, i8.q2, t2.f1, i4.f1
3539+
-> Seq Scan on public.text_tbl t2
3540+
Output: t2.f1
3541+
-> Materialize
3542+
Output: i8.q1, i8.q2, i4.f1, t1.f1
3543+
-> Nested Loop
3544+
Output: i8.q1, i8.q2, i4.f1, t1.f1
3545+
-> Nested Loop Left Join
3546+
Output: i8.q1, i8.q2, i4.f1
3547+
Join Filter: (i8.q1 = i4.f1)
3548+
-> Seq Scan on public.int8_tbl i8
3549+
Output: i8.q1, i8.q2
3550+
Filter: (i8.q2 = 456)
3551+
-> Seq Scan on public.int4_tbl i4
3552+
Output: i4.f1
3553+
-> Seq Scan on public.text_tbl t1
3554+
Output: t1.f1
3555+
Filter: (t1.f1 = 'doh!'::text)
3556+
(19 rows)
3557+
3558+
select * from
3559+
text_tbl t1
3560+
inner join int8_tbl i8
3561+
on i8.q2 = 456
3562+
right join text_tbl t2
3563+
on t1.f1 = 'doh!'
3564+
left join int4_tbl i4
3565+
on i8.q1 = i4.f1;
3566+
f1 | q1 | q2 | f1 | f1
3567+
------+-----+-----+-------------------+----
3568+
doh! | 123 | 456 | doh! |
3569+
doh! | 123 | 456 | hi de ho neighbor |
3570+
(2 rows)
3571+
35263572
--
35273573
-- test ability to push constants through outer join clauses
35283574
--

‎src/test/regress/sql/join.sql

+19
Original file line numberDiff line numberDiff line change
@@ -1092,6 +1092,25 @@ select t1.* from
10921092
left join int4_tbl i4
10931093
on (i8.q2 = i4.f1);
10941094

1095+
explain (verbose, costs off)
1096+
select * from
1097+
text_tbl t1
1098+
inner join int8_tbl i8
1099+
on i8.q2 = 456
1100+
right join text_tbl t2
1101+
on t1.f1 = 'doh!'
1102+
left join int4_tbl i4
1103+
on i8.q1 = i4.f1;
1104+
1105+
select * from
1106+
text_tbl t1
1107+
inner join int8_tbl i8
1108+
on i8.q2 = 456
1109+
right join text_tbl t2
1110+
on t1.f1 = 'doh!'
1111+
left join int4_tbl i4
1112+
on i8.q1 = i4.f1;
1113+
10951114
--
10961115
-- test ability to push constants through outer join clauses
10971116
--

0 commit comments

Comments
 (0)