diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 7d08a4b85ea..a8bb700ab50 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -26,7 +26,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" @@ -453,17 +452,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 e573c7aff7d..f8d5bee2960 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -58,6 +58,7 @@ #include "parser/parse_relation.h" #include "parser/parsetree.h" #include "partitioning/partdesc.h" +#include "rewrite/rewriteManip.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/rel.h" @@ -125,8 +126,18 @@ typedef struct * subquery belonging to a set operation */ } standard_qp_extra; +/* 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 void grouping_planner(PlannerInfo *root, double tuple_fraction, SetOperationStmt *setops); @@ -845,6 +856,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 @@ -1290,6 +1309,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 368f4bddb90..d641ca24481 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -358,12 +358,14 @@ build_subplan(PlannerInfo *root, Plan *plan, Path *path, * adjusted to have the correct varlevelsup, phlevelsup, or * agglevelsup. * - * If it's a PlaceHolderVar, Aggref or GroupingFunc, 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 or GroupingFunc, 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)) arg = SS_process_sublinks(root, arg, false); diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 9467083968c..8629cb2455a 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -2952,9 +2952,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: @@ -3658,20 +3657,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/var.c b/src/backend/optimizer/util/var.c index 844fc30978b..748944c5e27 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -857,12 +857,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, - (void *) 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, + (void *) 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 16ffa410f7c..9e31ffb8a4f 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -5780,6 +5780,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 0fc7787f200..fdf75c2acbd 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2117,6 +2117,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 e3f53309e73..d5314ce86d4 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3782,6 +3782,7 @@ post_parse_analyze_hook_type postprocess_result_function pqbool pqsigfunc +preprocess_subquery_phvs_context printQueryOpt printTableContent printTableFooter