diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index bb38768bd4358f72896e2d2c549bbd64dedcd24d..1b264a29e2962428ecba9b2d8cde58618c71b8df 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 f2beb410e734f143ef4a3bd27ad206138d63c2a4..acc4fb1a185c90ac73aeff0ae4c05f7346b0fdac 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 394cda32e572d45da7c2f7d72b1279c7067db2c5..9cfc56ea541d41cedd65b18d519120fb413e3932 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 19862f39be3222aca5ae4b6b95663a47d3ff5edb..8b65245b5107595da7e15d09d009446ca0d701c7 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); - - /* Mark the PHV as possibly needed at the qual's syntactic level */ - phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need, relids); + phinfo = find_placeholder_info(root, phv, true); - /* - * 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 cce9e1ef1b1433c03eda677ddf11adb42803c404..f112419fd5885e8a41e08014c7c0c4257de524c4 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 5261691af69ca2e1e015fb6a439c5aad15bfd8d2..69ba6b6923ff24f13dbc7e964a10dede7c390850 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 178214b4b9d3258e934c775e07784a21a11eb3e0..a54000b27b250ab445cfbe012966f19bb6b60608 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 3012031fce3209cb6b36f57786d53801a7e5611d..1be80b8aeb65e8672efb8b500970aea399e34bb7 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 --