From 4da6439bd8553059766011e2a42c6e39df08717f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 1 Sep 2012 13:56:14 -0400 Subject: [PATCH] Fix mark_placeholder_maybe_needed to handle LATERAL references. If a PlaceHolderVar contains a pulled-up LATERAL reference, its minimum possible evaluation level might be higher in the join tree than its original syntactic location. That in turn affects the ph_needed level for any contained PlaceHolderVars (that is, those PHVs had better propagate up the join tree at least to the evaluation level of the outer PHV). We got this mostly right, but mark_placeholder_maybe_needed() failed to account for the effect, and in consequence could leave the inner PHVs with ph_may_need less than what their ultimate ph_needed value will be. That's bad because it could lead to failure to select a join order that will allow evaluation of the inner PHV at a valid location. Fix that, and add an Assert that checks that we don't ever set ph_needed to more than ph_may_need. --- src/backend/optimizer/plan/initsplan.c | 5 ++- src/backend/optimizer/util/placeholder.c | 15 +++++++-- src/test/regress/expected/join.out | 40 ++++++++++++++++++++++++ src/test/regress/sql/join.sql | 11 +++++++ 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index f582d4067b..9565e2d607 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -199,10 +199,13 @@ add_vars_to_targetlist(PlannerInfo *root, List *vars, /* * If we are creating PlaceHolderInfos, mark them with the correct * maybe-needed locations. Otherwise, it's too late to change - * that. + * that, so we'd better not have set ph_needed to more than + * ph_may_need. */ if (create_new_ph) mark_placeholder_maybe_needed(root, phinfo, where_needed); + else + Assert(bms_is_subset(phinfo->ph_needed, phinfo->ph_may_need)); } else elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node)); diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c index 317a01940e..345a0277a5 100644 --- a/src/backend/optimizer/util/placeholder.c +++ b/src/backend/optimizer/util/placeholder.c @@ -250,6 +250,8 @@ void mark_placeholder_maybe_needed(PlannerInfo *root, PlaceHolderInfo *phinfo, Relids relids) { + Relids est_eval_level; + /* Mark the PHV as possibly needed at the given syntactic level */ phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need, relids); @@ -258,11 +260,18 @@ mark_placeholder_maybe_needed(PlannerInfo *root, PlaceHolderInfo *phinfo, * lower-level PHVs. We need to get those into the PlaceHolderInfo list, * but they aren't going to be needed where the outer PHV is referenced. * Rather, they'll be needed where the outer PHV is evaluated. We can - * estimate that (conservatively) as the syntactic location of the PHV's - * expression. Recurse to take care of any such PHVs. + * estimate that conservatively as the syntactic location of the PHV's + * expression, but not less than the level of any Vars it contains. + * (Normally the Vars would come from below the syntactic location anyway, + * but this might not be true if the PHV contains any LATERAL references.) */ + est_eval_level = bms_union(phinfo->ph_var->phrels, phinfo->ph_eval_at); + + /* Now recurse to take care of any such PHVs */ mark_placeholders_in_expr(root, (Node *) phinfo->ph_var->phexpr, - phinfo->ph_var->phrels); + est_eval_level); + + bms_free(est_eval_level); } /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 7c1ab44861..914a6fd844 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -3465,6 +3465,46 @@ select v.* from -4567890123456789 | (20 rows) +-- case requiring nested PlaceHolderVars +explain (verbose, costs off) +select * from + int8_tbl c left join ( + int8_tbl a left join (select q1, coalesce(q2,42) as x from int8_tbl b) ss1 + on a.q2 = ss1.q1 + cross join + lateral (select q1, coalesce(ss1.x,q2) as y from int8_tbl d) ss2 + ) on c.q2 = ss2.q1, + lateral (select ss2.y) ss3; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ + Nested Loop + Output: c.q1, c.q2, a.q1, a.q2, b.q1, (COALESCE(b.q2, 42::bigint)), d.q1, (COALESCE((COALESCE(b.q2, 42::bigint)), d.q2)), ((COALESCE((COALESCE(b.q2, 42::bigint)), d.q2))) + -> Hash Right Join + Output: c.q1, c.q2, a.q1, a.q2, b.q1, d.q1, (COALESCE(b.q2, 42::bigint)), (COALESCE((COALESCE(b.q2, 42::bigint)), d.q2)) + Hash Cond: (d.q1 = c.q2) + -> Nested Loop + Output: a.q1, a.q2, b.q1, d.q1, (COALESCE(b.q2, 42::bigint)), COALESCE((COALESCE(b.q2, 42::bigint)), d.q2) + -> Hash Left Join + Output: a.q1, a.q2, b.q1, (COALESCE(b.q2, 42::bigint)) + Hash Cond: (a.q2 = b.q1) + -> Seq Scan on public.int8_tbl a + Output: a.q1, a.q2 + -> Hash + Output: b.q1, (COALESCE(b.q2, 42::bigint)) + -> Seq Scan on public.int8_tbl b + Output: b.q1, COALESCE(b.q2, 42::bigint) + -> Materialize + Output: d.q1, d.q2 + -> Seq Scan on public.int8_tbl d + Output: d.q1, d.q2 + -> Hash + Output: c.q1, c.q2 + -> Seq Scan on public.int8_tbl c + Output: c.q1, c.q2 + -> Result + Output: (COALESCE((COALESCE(b.q2, 42::bigint)), d.q2)) +(26 rows) + -- test some error cases where LATERAL should have been used but wasn't select f1,g from int4_tbl a, generate_series(0, f1) g; ERROR: column "f1" does not exist diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 2213a446a3..fcc6572709 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -942,6 +942,17 @@ select v.* from left join int4_tbl z on z.f1 = x.q2, lateral (select x.q1,y.q1 from dual union all select x.q2,y.q2 from dual) v(vx,vy); +-- case requiring nested PlaceHolderVars +explain (verbose, costs off) +select * from + int8_tbl c left join ( + int8_tbl a left join (select q1, coalesce(q2,42) as x from int8_tbl b) ss1 + on a.q2 = ss1.q1 + cross join + lateral (select q1, coalesce(ss1.x,q2) as y from int8_tbl d) ss2 + ) on c.q2 = ss2.q1, + lateral (select ss2.y) ss3; + -- test some error cases where LATERAL should have been used but wasn't select f1,g from int4_tbl a, generate_series(0, f1) g; select f1,g from int4_tbl a, generate_series(0, a.f1) g; -- 2.40.0