From 77ba23256404d6f975a80d5402e62f6047c67b3a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 9 Aug 2011 00:48:51 -0400 Subject: [PATCH] Fix nested PlaceHolderVar expressions that appear only in targetlists. A PlaceHolderVar's expression might contain another, lower-level PlaceHolderVar. If the outer PlaceHolderVar is used, the inner one certainly will be also, and so we have to make sure that both of them get into the placeholder_list with correct ph_may_need values during the initial pre-scan of the query (before deconstruct_jointree starts). We did this correctly for PlaceHolderVars appearing in the query quals, but overlooked the issue for those appearing in the top-level targetlist; with the result that nested placeholders referenced only in the targetlist did not work correctly, as illustrated in bug #6154. While at it, add some error checking to find_placeholder_info to ensure that we don't try to create new placeholders after it's too late to do so; they have to all be created before deconstruct_jointree starts. Back-patch to 8.4 where the PlaceHolderVar mechanism was introduced. --- src/backend/optimizer/path/costsize.c | 2 +- src/backend/optimizer/path/equivclass.c | 2 +- src/backend/optimizer/plan/initsplan.c | 27 +++++--- src/backend/optimizer/util/placeholder.c | 84 ++++++++++++++++-------- src/include/optimizer/placeholder.h | 4 +- src/include/optimizer/planmain.h | 2 +- src/test/regress/expected/join.out | 45 +++++++++++++ src/test/regress/sql/join.sql | 37 +++++++++++ 8 files changed, 163 insertions(+), 40 deletions(-) diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index bb38768bd4..1b264a29e2 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -3491,7 +3491,7 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel) else if (IsA(node, PlaceHolderVar)) { PlaceHolderVar *phv = (PlaceHolderVar *) node; - PlaceHolderInfo *phinfo = find_placeholder_info(root, phv); + PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, false); tuple_width += phinfo->ph_width; } diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index f2beb410e7..acc4fb1a18 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -850,7 +850,7 @@ generate_base_implied_equalities_no_const(PlannerInfo *root, PVC_RECURSE_AGGREGATES, PVC_INCLUDE_PLACEHOLDERS); - add_vars_to_targetlist(root, vars, ec->ec_relids); + add_vars_to_targetlist(root, vars, ec->ec_relids, false); list_free(vars); } } diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 394cda32e5..9cfc56ea54 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -137,7 +137,7 @@ build_base_rel_tlists(PlannerInfo *root, List *final_tlist) if (tlist_vars != NIL) { - add_vars_to_targetlist(root, tlist_vars, bms_make_singleton(0)); + add_vars_to_targetlist(root, tlist_vars, bms_make_singleton(0), true); list_free(tlist_vars); } } @@ -151,10 +151,15 @@ build_base_rel_tlists(PlannerInfo *root, List *final_tlist) * * The list may also contain PlaceHolderVars. These don't necessarily * have a single owning relation; we keep their attr_needed info in - * root->placeholder_list instead. + * root->placeholder_list instead. If create_new_ph is true, it's OK + * to create new PlaceHolderInfos, and we also have to update ph_may_need; + * otherwise, the PlaceHolderInfos must already exist, and we should only + * update their ph_needed. (It should be true before deconstruct_jointree + * begins, and false after that.) */ void -add_vars_to_targetlist(PlannerInfo *root, List *vars, Relids where_needed) +add_vars_to_targetlist(PlannerInfo *root, List *vars, + Relids where_needed, bool create_new_ph) { ListCell *temp; @@ -185,18 +190,20 @@ add_vars_to_targetlist(PlannerInfo *root, List *vars, Relids where_needed) else if (IsA(node, PlaceHolderVar)) { PlaceHolderVar *phv = (PlaceHolderVar *) node; - PlaceHolderInfo *phinfo = find_placeholder_info(root, phv); + PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, + create_new_ph); + /* Always adjust ph_needed */ phinfo->ph_needed = bms_add_members(phinfo->ph_needed, where_needed); /* - * Update ph_may_need too. This is currently only necessary when - * being called from build_base_rel_tlists, but we may as well do - * it always. + * If we are creating PlaceHolderInfos, mark them with the + * correct maybe-needed locations. Otherwise, it's too late to + * change that. */ - phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need, - where_needed); + if (create_new_ph) + mark_placeholder_maybe_needed(root, phinfo, where_needed); } else elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node)); @@ -1035,7 +1042,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause, PVC_RECURSE_AGGREGATES, PVC_INCLUDE_PLACEHOLDERS); - add_vars_to_targetlist(root, vars, relids); + add_vars_to_targetlist(root, vars, relids, false); list_free(vars); } diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c index 19862f39be..8b65245b51 100644 --- a/src/backend/optimizer/util/placeholder.c +++ b/src/backend/optimizer/util/placeholder.c @@ -24,7 +24,7 @@ /* Local functions */ static Relids find_placeholders_recurse(PlannerInfo *root, Node *jtnode); -static void find_placeholders_in_qual(PlannerInfo *root, Node *qual, +static void mark_placeholders_in_expr(PlannerInfo *root, Node *expr, Relids relids); @@ -50,7 +50,10 @@ make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels) /* * find_placeholder_info - * Fetch the PlaceHolderInfo for the given PHV; create it if not found + * Fetch the PlaceHolderInfo for the given PHV + * + * If the PlaceHolderInfo doesn't exist yet, create it if create_new_ph is + * true, else throw an error. * * This is separate from make_placeholder_expr because subquery pullup has * to make PlaceHolderVars for expressions that might not be used at all in @@ -58,10 +61,13 @@ make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels) * We build PlaceHolderInfos only for PHVs that are still present in the * simplified query passed to query_planner(). * - * Note: this should only be called after query_planner() has started. + * Note: this should only be called after query_planner() has started. Also, + * create_new_ph must not be TRUE after deconstruct_jointree begins, because + * make_outerjoininfo assumes that we already know about all placeholders. */ PlaceHolderInfo * -find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv) +find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv, + bool create_new_ph) { PlaceHolderInfo *phinfo; ListCell *lc; @@ -77,6 +83,9 @@ find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv) } /* Not found, so create it */ + if (!create_new_ph) + elog(ERROR, "too late to create a new PlaceHolderInfo"); + phinfo = makeNode(PlaceHolderInfo); phinfo->phid = phv->phid; @@ -157,7 +166,7 @@ find_placeholders_recurse(PlannerInfo *root, Node *jtnode) /* * Now process the top-level quals. */ - find_placeholders_in_qual(root, f->quals, jtrelids); + mark_placeholders_in_expr(root, f->quals, jtrelids); } else if (IsA(jtnode, JoinExpr)) { @@ -173,7 +182,7 @@ find_placeholders_recurse(PlannerInfo *root, Node *jtnode) jtrelids = bms_join(leftids, rightids); /* Process the qual clauses */ - find_placeholders_in_qual(root, j->quals, jtrelids); + mark_placeholders_in_expr(root, j->quals, jtrelids); } else { @@ -185,14 +194,15 @@ find_placeholders_recurse(PlannerInfo *root, Node *jtnode) } /* - * find_placeholders_in_qual - * Process a qual clause for find_placeholders_in_jointree. + * mark_placeholders_in_expr + * Find all PlaceHolderVars in the given expression, and mark them + * as possibly needed at the specified join level. * * relids is the syntactic join level to mark as the "maybe needed" level - * for each PlaceHolderVar found in the qual clause. + * for each PlaceHolderVar found in the expression. */ static void -find_placeholders_in_qual(PlannerInfo *root, Node *qual, Relids relids) +mark_placeholders_in_expr(PlannerInfo *root, Node *expr, Relids relids) { List *vars; ListCell *vl; @@ -201,7 +211,7 @@ find_placeholders_in_qual(PlannerInfo *root, Node *qual, Relids relids) * pull_var_clause does more than we need here, but it'll do and it's * convenient to use. */ - vars = pull_var_clause(qual, + vars = pull_var_clause(expr, PVC_RECURSE_AGGREGATES, PVC_INCLUDE_PLACEHOLDERS); foreach(vl, vars) @@ -214,25 +224,47 @@ find_placeholders_in_qual(PlannerInfo *root, Node *qual, Relids relids) continue; /* Create a PlaceHolderInfo entry if there's not one already */ - phinfo = find_placeholder_info(root, phv); + phinfo = find_placeholder_info(root, phv, true); - /* Mark the PHV as possibly needed at the qual's syntactic level */ - phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need, relids); - - /* - * This is a bit tricky: the PHV's contained expression may contain - * other, 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. - */ - find_placeholders_in_qual(root, (Node *) phv->phexpr, phv->phrels); + /* Mark it, and recursively process any contained placeholders */ + mark_placeholder_maybe_needed(root, phinfo, relids); } list_free(vars); } +/* + * mark_placeholder_maybe_needed + * Mark a placeholder as possibly needed at the specified join level. + * + * relids is the syntactic join level to mark as the "maybe needed" level + * for the placeholder. + * + * This is called during an initial scan of the query's targetlist and quals + * before we begin deconstruct_jointree. Once we begin deconstruct_jointree, + * all active placeholders must be present in root->placeholder_list with + * their correct ph_may_need values, because make_outerjoininfo and + * update_placeholder_eval_levels require this info to be available while + * we crawl up the join tree. + */ +void +mark_placeholder_maybe_needed(PlannerInfo *root, PlaceHolderInfo *phinfo, + Relids relids) +{ + /* Mark the PHV as possibly needed at the given syntactic level */ + phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need, relids); + + /* + * This is a bit tricky: the PHV's contained expression may contain other, + * 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. + */ + mark_placeholders_in_expr(root, (Node *) phinfo->ph_var->phexpr, + phinfo->ph_var->phrels); +} + /* * update_placeholder_eval_levels * Adjust the target evaluation levels for placeholders @@ -353,7 +385,7 @@ fix_placeholder_input_needed_levels(PlannerInfo *root) PVC_RECURSE_AGGREGATES, PVC_INCLUDE_PLACEHOLDERS); - add_vars_to_targetlist(root, vars, eval_at); + add_vars_to_targetlist(root, vars, eval_at, false); list_free(vars); } } diff --git a/src/include/optimizer/placeholder.h b/src/include/optimizer/placeholder.h index cce9e1ef1b..f112419fd5 100644 --- a/src/include/optimizer/placeholder.h +++ b/src/include/optimizer/placeholder.h @@ -20,8 +20,10 @@ extern PlaceHolderVar *make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels); extern PlaceHolderInfo *find_placeholder_info(PlannerInfo *root, - PlaceHolderVar *phv); + PlaceHolderVar *phv, bool create_new_ph); extern void find_placeholders_in_jointree(PlannerInfo *root); +extern void mark_placeholder_maybe_needed(PlannerInfo *root, + PlaceHolderInfo *phinfo, Relids relids); extern void update_placeholder_eval_levels(PlannerInfo *root, SpecialJoinInfo *new_sjinfo); extern void fix_placeholder_input_needed_levels(PlannerInfo *root); diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h index 5261691af6..69ba6b6923 100644 --- a/src/include/optimizer/planmain.h +++ b/src/include/optimizer/planmain.h @@ -92,7 +92,7 @@ extern int join_collapse_limit; extern void add_base_rels_to_query(PlannerInfo *root, Node *jtnode); extern void build_base_rel_tlists(PlannerInfo *root, List *final_tlist); extern void add_vars_to_targetlist(PlannerInfo *root, List *vars, - Relids where_needed); + Relids where_needed, bool create_new_ph); extern List *deconstruct_jointree(PlannerInfo *root); extern void distribute_restrictinfo_to_rels(PlannerInfo *root, RestrictInfo *restrictinfo); diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 178214b4b9..a54000b27b 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2488,6 +2488,51 @@ order by c.name; (3 rows) rollback; +-- +-- test incorrect handling of placeholders that only appear in targetlists, +-- per bug #6154 +-- +SELECT * FROM +( SELECT 1 as key1 ) sub1 +LEFT JOIN +( SELECT sub3.key3, sub4.value2, COALESCE(sub4.value2, 66) as value3 FROM + ( SELECT 1 as key3 ) sub3 + LEFT JOIN + ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM + ( SELECT 1 as key5 ) sub5 + LEFT JOIN + ( SELECT 2 as key6, 42 as value1 ) sub6 + ON sub5.key5 = sub6.key6 + ) sub4 + ON sub4.key5 = sub3.key3 +) sub2 +ON sub1.key1 = sub2.key3; + key1 | key3 | value2 | value3 +------+------+--------+-------- + 1 | 1 | 1 | 1 +(1 row) + +-- test the path using join aliases, too +SELECT * FROM +( SELECT 1 as key1 ) sub1 +LEFT JOIN +( SELECT sub3.key3, value2, COALESCE(value2, 66) as value3 FROM + ( SELECT 1 as key3 ) sub3 + LEFT JOIN + ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM + ( SELECT 1 as key5 ) sub5 + LEFT JOIN + ( SELECT 2 as key6, 42 as value1 ) sub6 + ON sub5.key5 = sub6.key6 + ) sub4 + ON sub4.key5 = sub3.key3 +) sub2 +ON sub1.key1 = sub2.key3; + key1 | key3 | value2 | value3 +------+------+--------+-------- + 1 | 1 | 1 | 1 +(1 row) + -- -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 3012031fce..1be80b8aeb 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -602,6 +602,43 @@ order by c.name; rollback; +-- +-- test incorrect handling of placeholders that only appear in targetlists, +-- per bug #6154 +-- +SELECT * FROM +( SELECT 1 as key1 ) sub1 +LEFT JOIN +( SELECT sub3.key3, sub4.value2, COALESCE(sub4.value2, 66) as value3 FROM + ( SELECT 1 as key3 ) sub3 + LEFT JOIN + ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM + ( SELECT 1 as key5 ) sub5 + LEFT JOIN + ( SELECT 2 as key6, 42 as value1 ) sub6 + ON sub5.key5 = sub6.key6 + ) sub4 + ON sub4.key5 = sub3.key3 +) sub2 +ON sub1.key1 = sub2.key3; + +-- test the path using join aliases, too +SELECT * FROM +( SELECT 1 as key1 ) sub1 +LEFT JOIN +( SELECT sub3.key3, value2, COALESCE(value2, 66) as value3 FROM + ( SELECT 1 as key3 ) sub3 + LEFT JOIN + ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM + ( SELECT 1 as key5 ) sub5 + LEFT JOIN + ( SELECT 2 as key6, 42 as value1 ) sub6 + ON sub5.key5 = sub6.key6 + ) sub4 + ON sub4.key5 = sub3.key3 +) sub2 +ON sub1.key1 = sub2.key3; + -- -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE --