Skip to content

Commit b7d493b

Browse files
tglsfdctglsfdc
committed
Fix incorrect matching of subexpressions in outer-join plan nodes.
Previously we would re-use input subexpressions in all expression trees attached to a Join plan node. However, if it's an outer join and the subexpression appears in the nullable-side input, this is potentially incorrect for apparently-matching subexpressions that came from above the outer join (ie, targetlist and qpqual expressions), because the executor will treat the subexpression value as NULL when maybe it should not be. The case is fairly hard to hit because (a) you need a non-strict subexpression (else NULL is correct), and (b) we don't usually compute expressions in the outputs of non-toplevel plan nodes. But we might do so if the expressions are sort keys for a mergejoin, for example. Probably in the long run we should make a more explicit distinction between Vars appearing above and below an outer join, but that will be a major planner redesign and not at all back-patchable. For the moment, just hack set_join_references so that it will not match any non-Var expressions coming from nullable inputs to expressions that came from above the join. (This is somewhat overkill, in that a strict expression could still be matched, but it doesn't seem worth the effort to check that.) Per report from Qingqing Zhou. The added regression test case is based on his example. This has been broken for a very long time, so back-patch to all active branches.
1 parent 0aff9d8 commit b7d493b

File tree

3 files changed

+121
-14
lines changed

3 files changed

+121
-14
lines changed

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 53 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1091,19 +1091,13 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset)
10911091
outer_itlist = build_tlist_index(outer_plan->targetlist);
10921092
inner_itlist = build_tlist_index(inner_plan->targetlist);
10931093

1094-
/* All join plans have tlist, qual, and joinqual */
1095-
join->plan.targetlist = fix_join_expr(root,
1096-
join->plan.targetlist,
1097-
outer_itlist,
1098-
inner_itlist,
1099-
(Index) 0,
1100-
rtoffset);
1101-
join->plan.qual = fix_join_expr(root,
1102-
join->plan.qual,
1103-
outer_itlist,
1104-
inner_itlist,
1105-
(Index) 0,
1106-
rtoffset);
1094+
/*
1095+
* First process the joinquals (including merge or hash clauses). These
1096+
* are logically below the join so they can always use all values
1097+
* available from the input tlists. It's okay to also handle
1098+
* NestLoopParams now, because those couldn't refer to nullable
1099+
* subexpressions.
1100+
*/
11071101
join->joinqual = fix_join_expr(root,
11081102
join->joinqual,
11091103
outer_itlist,
@@ -1155,6 +1149,49 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset)
11551149
rtoffset);
11561150
}
11571151

1152+
/*
1153+
* Now we need to fix up the targetlist and qpqual, which are logically
1154+
* above the join. This means they should not re-use any input expression
1155+
* that was computed in the nullable side of an outer join. Vars and
1156+
* PlaceHolderVars are fine, so we can implement this restriction just by
1157+
* clearing has_non_vars in the indexed_tlist structs.
1158+
*
1159+
* XXX This is a grotty workaround for the fact that we don't clearly
1160+
* distinguish between a Var appearing below an outer join and the "same"
1161+
* Var appearing above it. If we did, we'd not need to hack the matching
1162+
* rules this way.
1163+
*/
1164+
switch (join->jointype)
1165+
{
1166+
case JOIN_LEFT:
1167+
case JOIN_SEMI:
1168+
case JOIN_ANTI:
1169+
inner_itlist->has_non_vars = false;
1170+
break;
1171+
case JOIN_RIGHT:
1172+
outer_itlist->has_non_vars = false;
1173+
break;
1174+
case JOIN_FULL:
1175+
outer_itlist->has_non_vars = false;
1176+
inner_itlist->has_non_vars = false;
1177+
break;
1178+
default:
1179+
break;
1180+
}
1181+
1182+
join->plan.targetlist = fix_join_expr(root,
1183+
join->plan.targetlist,
1184+
outer_itlist,
1185+
inner_itlist,
1186+
(Index) 0,
1187+
rtoffset);
1188+
join->plan.qual = fix_join_expr(root,
1189+
join->plan.qual,
1190+
outer_itlist,
1191+
inner_itlist,
1192+
(Index) 0,
1193+
rtoffset);
1194+
11581195
pfree(outer_itlist);
11591196
pfree(inner_itlist);
11601197
}
@@ -1433,7 +1470,9 @@ search_indexed_tlist_for_var(Var *var, indexed_tlist *itlist,
14331470
* If no match, return NULL.
14341471
*
14351472
* NOTE: it is a waste of time to call this unless itlist->has_ph_vars or
1436-
* itlist->has_non_vars
1473+
* itlist->has_non_vars. Furthermore, set_join_references() relies on being
1474+
* able to prevent matching of non-Vars by clearing itlist->has_non_vars,
1475+
* so there's a correctness reason not to call it unless that's set.
14371476
*/
14381477
static Var *
14391478
search_indexed_tlist_for_non_var(Node *node,

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3105,6 +3105,54 @@ explain (costs off)
31053105
Index Cond: (unique2 = 42)
31063106
(6 rows)
31073107

3108+
--
3109+
-- test that quals attached to an outer join have correct semantics,
3110+
-- specifically that they don't re-use expressions computed below the join;
3111+
-- we force a mergejoin so that coalesce(b.q1, 1) appears as a join input
3112+
--
3113+
set enable_hashjoin to off;
3114+
set enable_nestloop to off;
3115+
explain (verbose, costs off)
3116+
select a.q2, b.q1
3117+
from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
3118+
where coalesce(b.q1, 1) > 0;
3119+
QUERY PLAN
3120+
-------------------------------------------------------
3121+
Merge Left Join
3122+
Output: a.q2, b.q1
3123+
Merge Cond: (a.q2 = (COALESCE(b.q1, 1::bigint)))
3124+
Filter: (COALESCE(b.q1, 1::bigint) > 0)
3125+
-> Sort
3126+
Output: a.q2
3127+
Sort Key: a.q2
3128+
-> Seq Scan on public.int8_tbl a
3129+
Output: a.q2
3130+
-> Sort
3131+
Output: b.q1, (COALESCE(b.q1, 1::bigint))
3132+
Sort Key: (COALESCE(b.q1, 1::bigint))
3133+
-> Seq Scan on public.int8_tbl b
3134+
Output: b.q1, COALESCE(b.q1, 1::bigint)
3135+
(14 rows)
3136+
3137+
select a.q2, b.q1
3138+
from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
3139+
where coalesce(b.q1, 1) > 0;
3140+
q2 | q1
3141+
-------------------+------------------
3142+
-4567890123456789 |
3143+
123 | 123
3144+
123 | 123
3145+
456 |
3146+
4567890123456789 | 4567890123456789
3147+
4567890123456789 | 4567890123456789
3148+
4567890123456789 | 4567890123456789
3149+
4567890123456789 | 4567890123456789
3150+
4567890123456789 | 4567890123456789
3151+
4567890123456789 | 4567890123456789
3152+
(10 rows)
3153+
3154+
reset enable_hashjoin;
3155+
reset enable_nestloop;
31083156
--
31093157
-- test join removal
31103158
--

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,6 +907,26 @@ explain (costs off)
907907
explain (costs off)
908908
select * from tenk1 a full join tenk1 b using(unique2) where unique2 = 42;
909909

910+
--
911+
-- test that quals attached to an outer join have correct semantics,
912+
-- specifically that they don't re-use expressions computed below the join;
913+
-- we force a mergejoin so that coalesce(b.q1, 1) appears as a join input
914+
--
915+
916+
set enable_hashjoin to off;
917+
set enable_nestloop to off;
918+
919+
explain (verbose, costs off)
920+
select a.q2, b.q1
921+
from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
922+
where coalesce(b.q1, 1) > 0;
923+
select a.q2, b.q1
924+
from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
925+
where coalesce(b.q1, 1) > 0;
926+
927+
reset enable_hashjoin;
928+
reset enable_nestloop;
929+
910930
--
911931
-- test join removal
912932
--

0 commit comments

Comments
 (0)