diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 408de3a5907..e7d6ff1b687 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -27,7 +27,6 @@ #include "optimizer/paths.h" #include "optimizer/placeholder.h" #include "optimizer/planmain.h" -#include "optimizer/planner.h" #include "optimizer/restrictinfo.h" #include "parser/analyze.h" #include "rewrite/rewriteManip.h" @@ -804,17 +803,13 @@ extract_lateral_references(PlannerInfo *root, RelOptInfo *brel, Index rtindex) PlaceHolderVar *phv = (PlaceHolderVar *) node; int levelsup = phv->phlevelsup; - /* Have to work harder to adjust the contained expression too */ + /* + * Have to work harder to adjust the contained expression too. + * (Its expression has already been preprocessed by + * subquery_planner(), so we must not do that again here.) + */ if (levelsup != 0) IncrementVarSublevelsUp(node, -levelsup, 0); - - /* - * If we pulled the PHV out of a subquery RTE, its expression - * needs to be preprocessed. subquery_planner() already did this - * for level-zero PHVs in function and values RTEs, though. - */ - if (levelsup > 0) - phv->phexpr = preprocess_phv_expression(root, phv->phexpr); } else Assert(false); diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 28abddea637..ad8ceb4cb35 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -138,8 +138,18 @@ typedef struct Index group_rtindex; } having_grouping_ctx; +/* Context for preprocess_subquery_phvs_walker */ +typedef struct +{ + PlannerInfo *root; + int sublevels_up; +} preprocess_subquery_phvs_context; + /* Local functions */ static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind); +static void preprocess_subquery_phvs(PlannerInfo *root); +static bool preprocess_subquery_phvs_walker(Node *node, + preprocess_subquery_phvs_context *context); static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode); static Bitmapset *find_having_conflicts(Query *parse, Index group_rtindex); static Oid having_var_grouping_eqop(Var *var, void *context); @@ -895,6 +905,14 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root, */ root->hasHavingQual = (parse->havingQual != NULL); + /* + * Preprocess any PlaceHolderVars of our level that were pushed down into + * subqueries. This must happen before anything can consume those copies, + * in particular before SubLinks below are turned into SubPlans. + */ + if (root->glob->lastPHId != 0) + preprocess_subquery_phvs(root); + /* * Do expression preprocessing on targetlist and quals, as well as other * random expressions in the querytree. Note that we do not need to @@ -1522,6 +1540,77 @@ preprocess_phv_expression(PlannerInfo *root, Expr *expr) return (Expr *) preprocess_expression(root, (Node *) expr, EXPRKIND_PHV); } +/* + * preprocess_subquery_phvs + * Preprocess copies of this level's PlaceHolderVars that were pushed + * down into subqueries. + * + * When a subquery (a LATERAL RTE or a SubLink's subselect) references a + * pulled-up output that must be wrapped in a PlaceHolderVar, the PHV + * expression is pushed down into the subquery. The subquery's own planning + * leaves that copy alone, since it belongs to our level, so we need to + * preprocess it. We modify the PHVs in place, temporarily adjusting each to + * our level, and handle nested copies innermost-first. + */ +static void +preprocess_subquery_phvs(PlannerInfo *root) +{ + preprocess_subquery_phvs_context context; + + context.root = root; + context.sublevels_up = 0; + (void) query_tree_walker(root->parse, preprocess_subquery_phvs_walker, + &context, 0); +} + +static bool +preprocess_subquery_phvs_walker(Node *node, + preprocess_subquery_phvs_context *context) +{ + if (node == NULL) + return false; + if (IsA(node, Query)) + { + bool result; + + context->sublevels_up++; + result = query_tree_walker((Query *) node, + preprocess_subquery_phvs_walker, + context, 0); + context->sublevels_up--; + return result; + } + if (IsA(node, PlaceHolderVar)) + { + PlaceHolderVar *phv = (PlaceHolderVar *) node; + + /* Handle any nested copies within the expression first */ + (void) expression_tree_walker(node, preprocess_subquery_phvs_walker, + context); + + /* + * Is this a copy of one of our PHVs that is pushed down into a + * subquery? + */ + if (context->sublevels_up > 0 && + phv->phlevelsup == context->sublevels_up) + { + int levelsup = phv->phlevelsup; + Node *expr; + + /* Adjust the expression to our level, preprocess, adjust back */ + expr = copyObject((Node *) phv->phexpr); + IncrementVarSublevelsUp(expr, -levelsup, 0); + expr = preprocess_expression(context->root, expr, EXPRKIND_PHV); + IncrementVarSublevelsUp(expr, levelsup, 0); + phv->phexpr = (Expr *) expr; + } + return false; + } + return expression_tree_walker(node, preprocess_subquery_phvs_walker, + context); +} + /*-------------------- * grouping_planner * Perform planning steps related to grouping, aggregation, etc. diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 23eace9dbc4..614fb3e6b9b 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -358,13 +358,14 @@ build_subplan(PlannerInfo *root, Plan *plan, Path *path, * already been adjusted to have the correct varlevelsup, phlevelsup, * agglevelsup, or retlevelsup. * - * If it's a PlaceHolderVar, Aggref, GroupingFunc, or ReturningExpr, - * its arguments might contain SubLinks, which have not yet been - * processed (see the comments for SS_replace_correlation_vars). Do - * that now. + * If it's an Aggref, GroupingFunc, or ReturningExpr, its arguments + * might contain SubLinks, which have not yet been processed (see the + * comments for SS_replace_correlation_vars). Do that now. A + * PlaceHolderVar needs no such treatment: subquery_planner already + * preprocessed the PHVs of its owning level, so its expression is + * fully processed and may already contain SubPlans. */ - if (IsA(arg, PlaceHolderVar) || - IsA(arg, Aggref) || + if (IsA(arg, Aggref) || IsA(arg, GroupingFunc) || IsA(arg, ReturningExpr)) arg = SS_process_sublinks(root, arg, false); diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 6a6a54498b7..ee89b5e7238 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -2973,9 +2973,8 @@ eval_const_expressions_mutator(Node *node, /* * Return a SubPlan unchanged --- too late to do anything with it. - * - * XXX should we ereport() here instead? Probably this routine - * should never be invoked after SubPlan creation. + * This can happen in estimation mode, which runs after SubPlans + * have been created. */ return node; case T_RelabelType: @@ -3681,20 +3680,28 @@ eval_const_expressions_mutator(Node *node, return (Node *) newcdomain; } case T_PlaceHolderVar: - - /* - * In estimation mode, just strip the PlaceHolderVar node - * altogether; this amounts to estimating that the contained value - * won't be forced to null by an outer join. In regular mode we - * just use the default behavior (ie, simplify the expression but - * leave the PlaceHolderVar node intact). - */ - if (context->estimate) { PlaceHolderVar *phv = (PlaceHolderVar *) node; - return eval_const_expressions_mutator((Node *) phv->phexpr, - context); + /* + * Leave a PHV of an upper query level alone: its expression + * belongs to that level, which has already preprocessed it. + * But we do copy the subtree, just to conform to this + * function's API spec. + */ + if (phv->phlevelsup > 0) + return copyObject(node); + + /* + * In estimation mode, just strip the PlaceHolderVar node + * altogether; this amounts to estimating that the contained + * value won't be forced to null by an outer join. In regular + * mode we just use the default behavior (ie, simplify the + * expression but leave the PlaceHolderVar node intact). + */ + if (context->estimate) + return eval_const_expressions_mutator((Node *) phv->phexpr, + context); } break; case T_ConvertRowtypeExpr: diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c index 4c13c5931b4..2fda7b3c8f6 100644 --- a/src/backend/optimizer/util/paramassign.c +++ b/src/backend/optimizer/util/paramassign.c @@ -677,19 +677,18 @@ identify_current_nestloop_params(PlannerInfo *root, /* * Deal with an edge case: if the PHV was pulled up out of a * subquery and it contains a subquery that was originally - * pushed down from this query level, then that will still be - * represented as a SubLink, because SS_process_sublinks won't - * recurse into outer PHVs, so it didn't get transformed - * during expression preprocessing in the subquery. We need a - * version of the PHV that has a SubPlan, which we can get - * from the current query level's placeholder_list. This is - * quite grotty of course, but dealing with it earlier in the - * handling of subplan params would be just as grotty, and it - * might end up being a waste of cycles if we don't decide to - * treat the PHV as a NestLoopParam. (Perhaps that whole - * mechanism should be redesigned someday, but today is not - * that day.) + * pushed down from this query level, then the copy of the PHV + * we got from the subquery has its own SubPlan for that, + * distinct from the one in the current query level's + * placeholder_list. We'd rather use the placeholder_list's + * version of the PHV, so that we don't end up evaluating + * duplicate SubPlans. + * + * Note that the subquery's copy cannot contain any SubLinks: + * subquery_planner preprocessed the PHVs of this query level + * within the subquery before the subquery was planned. */ + Assert(!checkExprHasSubLink((Node *) phv)); if (root->parse->hasSubLinks) { phv = copyObject(phinfo->ph_var); diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 9428cf5bb9c..af0cd7b4130 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -902,12 +902,35 @@ flatten_join_alias_vars_mutator(Node *node, } if (IsA(node, PlaceHolderVar)) { - /* Copy the PlaceHolderVar node with correct mutation of subnodes */ - PlaceHolderVar *phv; + PlaceHolderVar *phv = (PlaceHolderVar *) node; - phv = (PlaceHolderVar *) expression_tree_mutator(node, - flatten_join_alias_vars_mutator, - context); + /* + * A PHV above the target level can't contain join aliases of the + * target level and has no relid sets of ours to fix, so return it + * unchanged. + */ + if (phv->phlevelsup > context->sublevels_up) + return node; /* no need to copy, really */ + + /* + * A pushed-down copy of a target-level PHV was already preprocessed + * by preprocess_subquery_phvs, so it may contain SubPlans and holds + * no unflattened target-level aliases; shallow-copy it instead of + * recursing. Any other PHV is recursed into, as it may still hold + * target-level aliases. + */ + if (phv->phlevelsup == context->sublevels_up && + context->sublevels_up > 0) + { + PlaceHolderVar *newphv = makeNode(PlaceHolderVar); + + memcpy(newphv, phv, sizeof(PlaceHolderVar)); + phv = newphv; + } + else + phv = (PlaceHolderVar *) expression_tree_mutator(node, + flatten_join_alias_vars_mutator, + context); /* now fix PlaceHolderVar's relid sets */ if (phv->phlevelsup == context->sublevels_up) { diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 1a82a765ba7..980e26e1f1e 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -4318,7 +4318,7 @@ where i8.q2 = 123; QUERY PLAN ---------------------------------------------------------------- Nested Loop - Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2) + Output: i8.q1, i8.q2, (InitPlan 2).col1, false, (i8.q2) InitPlan 1 -> Result Output: true @@ -4331,7 +4331,7 @@ where i8.q2 = 123; -> Nested Loop Output: i8.q1, i8.q2, (i8.q2) -> Subquery Scan on ss1 - Output: ss1.y, (InitPlan 1).col1 + Output: ss1.y, (InitPlan 2).col1 -> Limit Output: NULL::integer -> Result @@ -4345,7 +4345,7 @@ where i8.q2 = 123; Output: (i8.q2) -> Result Output: i8.q2 - One-Time Filter: ((InitPlan 1).col1) + One-Time Filter: ((InitPlan 2).col1) (29 rows) explain (verbose, costs off) @@ -4361,7 +4361,7 @@ where i8.q2 = 123; QUERY PLAN ---------------------------------------------------------------- Nested Loop - Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2) + Output: i8.q1, i8.q2, (InitPlan 2).col1, false, (i8.q2) InitPlan 1 -> Result Output: true @@ -4375,7 +4375,7 @@ where i8.q2 = 123; -> Nested Loop Output: i8.q1, i8.q2, (i8.q2) -> Seq Scan on public.int4_tbl i4 - Output: i4.f1, (InitPlan 1).col1 + Output: i4.f1, (InitPlan 2).col1 Filter: (i4.f1 = 0) -> Nested Loop Output: i8.q1, i8.q2, (i8.q2) @@ -4386,7 +4386,7 @@ where i8.q2 = 123; Output: (i8.q2) -> Result Output: i8.q2 - One-Time Filter: ((InitPlan 1).col1) + One-Time Filter: ((InitPlan 2).col1) (27 rows) -- Test proper handling of appendrel PHVs during useless-RTE removal @@ -6271,6 +6271,151 @@ on lhs.id = rhs.id; -> Result (5 rows) +-- check handling of a removed Var that's pushed down into a subquery +-- (fallout from the fix for bug #19560) +explain (verbose, costs off) +select c1, c2 +from (select case when false then remov.id end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on false, + lateral (select ss1.c1 as c2 from int4_tbl i43 offset 0) ss2; + QUERY PLAN +---------------------------------------------- + Nested Loop + Output: (NULL::integer), ((NULL::integer)) + -> Nested Loop Left Join + Output: (NULL::integer) + Join Filter: false + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Result + Output: NULL::integer + One-Time Filter: false + -> Memoize + Output: ((NULL::integer)) + Cache Key: (NULL::integer) + Cache Mode: binary + -> Seq Scan on public.int4_tbl i43 + Output: (NULL::integer) +(16 rows) + +-- likewise, where the subquery is a UNION ALL whose arms are appendrel +-- children that are not simple enough to be pulled up +explain (verbose, costs off) +select c1, c2 +from (select case when false then remov.id end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on false, + lateral ((select ss1.c1 as c2 from int4_tbl i43 offset 0) + union all + (select ss1.c1 from int4_tbl i44 offset 0)) ss2; + QUERY PLAN +--------------------------------------------------- + Nested Loop + Output: (NULL::integer), ((NULL::integer)) + -> Nested Loop Left Join + Output: (NULL::integer) + Join Filter: false + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Result + Output: NULL::integer + One-Time Filter: false + -> Memoize + Output: ((NULL::integer)) + Cache Key: (NULL::integer) + Cache Mode: binary + -> Append + -> Seq Scan on public.int4_tbl i43 + Output: (NULL::integer) + -> Seq Scan on public.int4_tbl i44 + Output: (NULL::integer) +(19 rows) + +-- likewise, where the PHV contains a SubPlan and the subquery has a join, so +-- that its planner runs flatten_join_alias_vars over the outer-level PHV +explain (verbose, costs off) +select c1, c2 +from (select case when false then remov.id else (select i41.f1) end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on false, + lateral (select ss1.c1 as c2 from int4_tbl i43 join int4_tbl i44 on true + offset 0) ss2; + QUERY PLAN +--------------------------------------------------------- + Nested Loop + Output: ((SubPlan 2)), (((SubPlan 2))) + -> Nested Loop Left Join + Output: ((SubPlan 2)) + Join Filter: false + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Result + Output: (SubPlan 2) + One-Time Filter: false + SubPlan 2 + -> Result + Output: f1 + -> Memoize + Output: (((SubPlan 2))) + Cache Key: ((SubPlan 2)) + Cache Mode: binary + -> Nested Loop + Output: ((SubPlan 2)) + -> Seq Scan on public.int4_tbl i43 + Output: i43.f1 + -> Materialize + -> Seq Scan on public.int4_tbl i44 +(23 rows) + +-- likewise, where the PHV copy is pushed into a SubLink's subselect rather +-- than a LATERAL subquery +explain (verbose, costs off) +select (select ss1.c1 from int4_tbl i43 offset 0) as c2 +from (select case when false then remov.id end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on true; + QUERY PLAN +--------------------------------------------- + Nested Loop Left Join + Output: (SubPlan 1) + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Materialize + Output: (NULL::integer) + -> Seq Scan on public.int4_tbl i41 + Output: NULL::integer + SubPlan 1 + -> Seq Scan on public.int4_tbl i43 + Output: (NULL::integer) +(11 rows) + +-- likewise, where the pushed-down PHV's expression itself contains a SubLink, +-- so it must be preprocessed once at the outer level rather than again while +-- building the SubPlan that references it +explain (verbose, costs off) +select (select ss1.c1 from int4_tbl i43 offset 0) as c2 +from (select case when false then remov.id else (select i41.f1) end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on true; + QUERY PLAN +--------------------------------------------- + Nested Loop Left Join + Output: (SubPlan 2) + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Materialize + Output: ((SubPlan 1)) + -> Seq Scan on public.int4_tbl i41 + Output: (SubPlan 1) + SubPlan 1 + -> Result + Output: i41.f1 + SubPlan 2 + -> Seq Scan on public.int4_tbl i43 + Output: ((SubPlan 1)) +(14 rows) + -- More tests of correct placement of pseudoconstant quals -- simple constant-false condition explain (costs off) diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 2c7a360cf22..400b8bc3d25 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2303,6 +2303,53 @@ full join ) as rhs on lhs.id = rhs.id; +-- check handling of a removed Var that's pushed down into a subquery +-- (fallout from the fix for bug #19560) +explain (verbose, costs off) +select c1, c2 +from (select case when false then remov.id end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on false, + lateral (select ss1.c1 as c2 from int4_tbl i43 offset 0) ss2; + +-- likewise, where the subquery is a UNION ALL whose arms are appendrel +-- children that are not simple enough to be pulled up +explain (verbose, costs off) +select c1, c2 +from (select case when false then remov.id end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on false, + lateral ((select ss1.c1 as c2 from int4_tbl i43 offset 0) + union all + (select ss1.c1 from int4_tbl i44 offset 0)) ss2; + +-- likewise, where the PHV contains a SubPlan and the subquery has a join, so +-- that its planner runs flatten_join_alias_vars over the outer-level PHV +explain (verbose, costs off) +select c1, c2 +from (select case when false then remov.id else (select i41.f1) end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on false, + lateral (select ss1.c1 as c2 from int4_tbl i43 join int4_tbl i44 on true + offset 0) ss2; + +-- likewise, where the PHV copy is pushed into a SubLink's subselect rather +-- than a LATERAL subquery +explain (verbose, costs off) +select (select ss1.c1 from int4_tbl i43 offset 0) as c2 +from (select case when false then remov.id end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on true; + +-- likewise, where the pushed-down PHV's expression itself contains a SubLink, +-- so it must be preprocessed once at the outer level rather than again while +-- building the SubPlan that references it +explain (verbose, costs off) +select (select ss1.c1 from int4_tbl i43 offset 0) as c2 +from (select case when false then remov.id else (select i41.f1) end as c1 + from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 + right join int4_tbl i42 on true; + -- More tests of correct placement of pseudoconstant quals -- simple constant-false condition diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index b6f0897dc84..3ba0c7917bc 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3952,6 +3952,7 @@ post_parse_analyze_hook_type postprocess_result_function pqbool pqsigfunc +preprocess_subquery_phvs_context printQueryOpt printTableContent printTableFooter