From b3ff6c742f6c7f750e9f74476576839cb039e1ab Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 17 Aug 2022 15:52:53 -0400 Subject: [PATCH] Use an explicit state flag to control PlaceHolderInfo creation. Up to now, callers of find_placeholder_info() were required to pass a flag indicating if it's OK to make a new PlaceHolderInfo. That'd be fine if the callers had free choice, but they do not. Once we begin deconstruct_jointree() it's no longer OK to make more PHIs; while callers before that always want to create a PHI if it's not there already. So there's no freedom of action, only the opportunity to cause bugs by creating PHIs too late. Let's get rid of that in favor of adding a state flag PlannerInfo.placeholdersFrozen, which we can set at the point where it's no longer OK to make more PHIs. This patch also simplifies a couple of call sites that were using complicated logic to avoid calling find_placeholder_info() as much as possible. Now that that lookup is O(1) thanks to the previous commit, the extra bitmap manipulations are probably a net negative. Discussion: https://postgr.es/m/1405792.1660677844@sss.pgh.pa.us --- src/backend/optimizer/path/costsize.c | 2 +- src/backend/optimizer/path/equivclass.c | 2 +- src/backend/optimizer/plan/createplan.c | 9 ++----- src/backend/optimizer/plan/initsplan.c | 32 +++++++++++++---------- src/backend/optimizer/plan/planner.c | 1 + src/backend/optimizer/prep/prepjointree.c | 1 + src/backend/optimizer/util/inherit.c | 2 +- src/backend/optimizer/util/paramassign.c | 10 +++---- src/backend/optimizer/util/placeholder.c | 26 +++++++----------- src/include/nodes/pathnodes.h | 2 ++ src/include/optimizer/placeholder.h | 2 +- src/include/optimizer/planmain.h | 2 +- 12 files changed, 42 insertions(+), 49 deletions(-) diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 7292d68dbc..1e94c5aa7c 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -6245,7 +6245,7 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel) * scanning this rel, so be sure to include it in reltarget->cost. */ PlaceHolderVar *phv = (PlaceHolderVar *) node; - PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, false); + PlaceHolderInfo *phinfo = find_placeholder_info(root, phv); QualCost cost; tuple_width += phinfo->ph_width; diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index 7991295548..2c900e6b12 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -1323,7 +1323,7 @@ generate_base_implied_equalities_no_const(PlannerInfo *root, PVC_RECURSE_WINDOWFUNCS | PVC_INCLUDE_PLACEHOLDERS); - add_vars_to_targetlist(root, vars, ec->ec_relids, false); + add_vars_to_targetlist(root, vars, ec->ec_relids); list_free(vars); } } diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index aa9d4e51b9..cd8a3ef7cb 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -4915,13 +4915,8 @@ replace_nestloop_params_mutator(Node *node, PlannerInfo *root) /* Upper-level PlaceHolderVars should be long gone at this point */ Assert(phv->phlevelsup == 0); - /* - * Check whether we need to replace the PHV. We use bms_overlap as a - * cheap/quick test to see if the PHV might be evaluated in the outer - * rels, and then grab its PlaceHolderInfo to tell for sure. - */ - if (!bms_overlap(phv->phrels, root->curOuterRels) || - !bms_is_subset(find_placeholder_info(root, phv, false)->ph_eval_at, + /* Check whether we need to replace the PHV */ + if (!bms_is_subset(find_placeholder_info(root, phv)->ph_eval_at, root->curOuterRels)) { /* diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 023efbaf09..fd8cbb1dc7 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -189,7 +189,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), true); + add_vars_to_targetlist(root, tlist_vars, bms_make_singleton(0)); list_free(tlist_vars); } @@ -206,7 +206,7 @@ build_base_rel_tlists(PlannerInfo *root, List *final_tlist) if (having_vars != NIL) { add_vars_to_targetlist(root, having_vars, - bms_make_singleton(0), true); + bms_make_singleton(0)); list_free(having_vars); } } @@ -221,14 +221,12 @@ 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. If create_new_ph is true, it's OK - * to create new PlaceHolderInfos; otherwise, the PlaceHolderInfos must - * already exist, and we should only update their ph_needed. (This should - * be true before deconstruct_jointree begins, and false after that.) + * root->placeholder_list instead. Find or create the associated + * PlaceHolderInfo entry, and update its ph_needed. */ void add_vars_to_targetlist(PlannerInfo *root, List *vars, - Relids where_needed, bool create_new_ph) + Relids where_needed) { ListCell *temp; @@ -262,8 +260,7 @@ add_vars_to_targetlist(PlannerInfo *root, List *vars, else if (IsA(node, PlaceHolderVar)) { PlaceHolderVar *phv = (PlaceHolderVar *) node; - PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, - create_new_ph); + PlaceHolderInfo *phinfo = find_placeholder_info(root, phv); phinfo->ph_needed = bms_add_members(phinfo->ph_needed, where_needed); @@ -432,7 +429,7 @@ extract_lateral_references(PlannerInfo *root, RelOptInfo *brel, Index rtindex) * Push Vars into their source relations' targetlists, and PHVs into * root->placeholder_list. */ - add_vars_to_targetlist(root, newvars, where_needed, true); + add_vars_to_targetlist(root, newvars, where_needed); /* Remember the lateral references for create_lateral_join_info */ brel->lateral_vars = newvars; @@ -493,8 +490,7 @@ create_lateral_join_info(PlannerInfo *root) else if (IsA(node, PlaceHolderVar)) { PlaceHolderVar *phv = (PlaceHolderVar *) node; - PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, - false); + PlaceHolderInfo *phinfo = find_placeholder_info(root, phv); found_laterals = true; lateral_relids = bms_add_members(lateral_relids, @@ -691,6 +687,14 @@ deconstruct_jointree(PlannerInfo *root) Relids inner_join_rels; List *postponed_qual_list = NIL; + /* + * After this point, no more PlaceHolderInfos may be made, because + * make_outerjoininfo and update_placeholder_eval_levels require all + * active placeholders to be present in root->placeholder_list while we + * crawl up the join tree. + */ + root->placeholdersFrozen = true; + /* Start recursion at top of jointree */ Assert(root->parse->jointree != NULL && IsA(root->parse->jointree, FromExpr)); @@ -1866,7 +1870,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause, PVC_RECURSE_WINDOWFUNCS | PVC_INCLUDE_PLACEHOLDERS); - add_vars_to_targetlist(root, vars, relids, false); + add_vars_to_targetlist(root, vars, relids); list_free(vars); } @@ -2380,7 +2384,7 @@ process_implied_equality(PlannerInfo *root, PVC_RECURSE_WINDOWFUNCS | PVC_INCLUDE_PLACEHOLDERS); - add_vars_to_targetlist(root, vars, relids, false); + add_vars_to_targetlist(root, vars, relids); list_free(vars); } diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index e636596b57..bf45b498ba 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -635,6 +635,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse, root->qual_security_level = 0; root->hasPseudoConstantQuals = false; root->hasAlternativeSubPlans = false; + root->placeholdersFrozen = false; root->hasRecursion = hasRecursion; if (hasRecursion) root->wt_param_id = assign_special_exec_param(root); diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 0bd99acf83..41c7066d90 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -1011,6 +1011,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte, subroot->grouping_map = NULL; subroot->minmax_aggs = NIL; subroot->qual_security_level = 0; + subroot->placeholdersFrozen = false; subroot->hasRecursion = false; subroot->wt_param_id = -1; subroot->non_recursive_path = NULL; diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c index 7e134822f3..cf7691a474 100644 --- a/src/backend/optimizer/util/inherit.c +++ b/src/backend/optimizer/util/inherit.c @@ -291,7 +291,7 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel, * Add the newly added Vars to parent's reltarget. We needn't worry * about the children's reltargets, they'll be made later. */ - add_vars_to_targetlist(root, newvars, bms_make_singleton(0), false); + add_vars_to_targetlist(root, newvars, bms_make_singleton(0)); } table_close(oldrelation, NoLock); diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c index 12486cb067..8e2d4bf515 100644 --- a/src/backend/optimizer/util/paramassign.c +++ b/src/backend/optimizer/util/paramassign.c @@ -470,7 +470,7 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params) ListCell *lc; /* If not from a nestloop outer rel, complain */ - if (!bms_is_subset(find_placeholder_info(root, phv, false)->ph_eval_at, + if (!bms_is_subset(find_placeholder_info(root, phv)->ph_eval_at, root->curOuterRels)) elog(ERROR, "non-LATERAL parameter required by subquery"); @@ -517,8 +517,7 @@ identify_current_nestloop_params(PlannerInfo *root, Relids leftrelids) /* * We are looking for Vars and PHVs that can be supplied by the - * lefthand rels. The "bms_overlap" test is just an optimization to - * allow skipping find_placeholder_info() if the PHV couldn't match. + * lefthand rels. */ if (IsA(nlp->paramval, Var) && bms_is_member(nlp->paramval->varno, leftrelids)) @@ -528,11 +527,8 @@ identify_current_nestloop_params(PlannerInfo *root, Relids leftrelids) result = lappend(result, nlp); } else if (IsA(nlp->paramval, PlaceHolderVar) && - bms_overlap(((PlaceHolderVar *) nlp->paramval)->phrels, - leftrelids) && bms_is_subset(find_placeholder_info(root, - (PlaceHolderVar *) nlp->paramval, - false)->ph_eval_at, + (PlaceHolderVar *) nlp->paramval)->ph_eval_at, leftrelids)) { root->curOuterParams = foreach_delete_current(root->curOuterParams, diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c index 0184e6d36c..8a114fe946 100644 --- a/src/backend/optimizer/util/placeholder.c +++ b/src/backend/optimizer/util/placeholder.c @@ -52,8 +52,8 @@ make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels) * find_placeholder_info * 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. + * If the PlaceHolderInfo doesn't exist yet, create it if we haven't yet + * frozen the set of PlaceHolderInfos for the query; 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 @@ -61,13 +61,10 @@ 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. Also, - * create_new_ph must not be true after deconstruct_jointree begins, because - * make_outerjoininfo assumes that we already know about all placeholders. + * Note: this should only be called after query_planner() has started. */ PlaceHolderInfo * -find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv, - bool create_new_ph) +find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv) { PlaceHolderInfo *phinfo; Relids rels_used; @@ -87,7 +84,7 @@ find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv, } /* Not found, so create it */ - if (!create_new_ph) + if (root->placeholdersFrozen) elog(ERROR, "too late to create a new PlaceHolderInfo"); phinfo = makeNode(PlaceHolderInfo); @@ -166,16 +163,13 @@ find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv, * * We don't need to look at the targetlist because build_base_rel_tlists() * will already have made entries for any PHVs in the tlist. - * - * This is called before we begin deconstruct_jointree. Once we begin - * deconstruct_jointree, all active placeholders must be present in - * root->placeholder_list, because make_outerjoininfo and - * update_placeholder_eval_levels require this info to be available - * while we crawl up the join tree. */ void find_placeholders_in_jointree(PlannerInfo *root) { + /* This must be done before freezing the set of PHIs */ + Assert(!root->placeholdersFrozen); + /* We need do nothing if the query contains no PlaceHolderVars */ if (root->glob->lastPHId != 0) { @@ -265,7 +259,7 @@ find_placeholders_in_expr(PlannerInfo *root, Node *expr) continue; /* Create a PlaceHolderInfo entry if there's not one already */ - (void) find_placeholder_info(root, phv, true); + (void) find_placeholder_info(root, phv); } list_free(vars); } @@ -392,7 +386,7 @@ fix_placeholder_input_needed_levels(PlannerInfo *root) PVC_RECURSE_WINDOWFUNCS | PVC_INCLUDE_PLACEHOLDERS); - add_vars_to_targetlist(root, vars, phinfo->ph_eval_at, false); + add_vars_to_targetlist(root, vars, phinfo->ph_eval_at); list_free(vars); } } diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index b310d3c6fd..bdc7b50db9 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -454,6 +454,8 @@ struct PlannerInfo bool hasPseudoConstantQuals; /* true if we've made any of those */ bool hasAlternativeSubPlans; + /* true once we're no longer allowed to add PlaceHolderInfos */ + bool placeholdersFrozen; /* true if planning a recursive WITH item */ bool hasRecursion; diff --git a/src/include/optimizer/placeholder.h b/src/include/optimizer/placeholder.h index 39803ea41f..507dbc6175 100644 --- a/src/include/optimizer/placeholder.h +++ b/src/include/optimizer/placeholder.h @@ -20,7 +20,7 @@ extern PlaceHolderVar *make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels); extern PlaceHolderInfo *find_placeholder_info(PlannerInfo *root, - PlaceHolderVar *phv, bool create_new_ph); + PlaceHolderVar *phv); extern void find_placeholders_in_jointree(PlannerInfo *root); extern void update_placeholder_eval_levels(PlannerInfo *root, SpecialJoinInfo *new_sjinfo); diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h index c4f61c1a09..1566f435b3 100644 --- a/src/include/optimizer/planmain.h +++ b/src/include/optimizer/planmain.h @@ -71,7 +71,7 @@ extern void add_base_rels_to_query(PlannerInfo *root, Node *jtnode); extern void add_other_rels_to_query(PlannerInfo *root); extern void build_base_rel_tlists(PlannerInfo *root, List *final_tlist); extern void add_vars_to_targetlist(PlannerInfo *root, List *vars, - Relids where_needed, bool create_new_ph); + Relids where_needed); extern void find_lateral_references(PlannerInfo *root); extern void create_lateral_join_info(PlannerInfo *root); extern List *deconstruct_jointree(PlannerInfo *root);