From 10bbab6257e742aa80b298733be6aba34623c20c Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Sun, 30 Aug 2026 08:21:21 +0900 Subject: [PATCH v2] Fix stale copies of PHVs in LATERAL subqueries When a LATERAL subquery references an output of another subquery that gets pulled up, and that output must be wrapped in a PlaceHolderVar because of an intermediate outer join, the PHV expression is pushed down into the LATERAL subquery. That copy is not preprocessed along with the outer query's expressions, since planning of the subquery happens later. So the two copies can diverge. That used to be harmless, because the subquery's copy is never evaluated and is matched to the outer PHV by phid only. But since commit 2ebf25e7d, join removal edits the whole query tree to remove references to the removed rel, and it walks into subqueries. If join removal deletes the rel whose Var survives only in the subquery's copy, we hit an Assert in ChangeVarNodes. To fix, preprocess the PHV copies within each LATERAL subquery RTE in subquery_planner, right where we already flatten join alias Vars in such subqueries, so that they match the preprocessed PHVs of the outer query. extract_lateral_references no longer needs to preprocess the copies it pulls out of subqueries, and it must not, since preprocessing is not something we can safely do twice on the same expression. One consequence is that a not-yet-planned subquery can now contain SubPlans, within a PHV of an upper query level. Teach flatten_join_alias_vars_mutator to leave such PHVs alone, as process_sublinks_mutator already does; a PHV of an upper level cannot contain join aliases of the level being flattened anyway. Also update the comment in identify_current_nestloop_params, since the subquery's copy of a PHV can no longer contain unprocessed SubLinks. We still prefer the PlaceHolderInfo's copy there, to avoid evaluating duplicate SubPlans. --- src/backend/optimizer/plan/initsplan.c | 15 ++-- src/backend/optimizer/plan/planner.c | 53 ++++++++++-- src/backend/optimizer/util/paramassign.c | 23 +++--- src/backend/optimizer/util/var.c | 10 ++- src/include/optimizer/planner.h | 1 - src/test/regress/expected/join.out | 100 +++++++++++++++++++++++ src/test/regress/sql/join.sql | 30 +++++++ 7 files changed, 199 insertions(+), 33 deletions(-) diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index a2fb0b55a79..9b73c0dc771 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -29,7 +29,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" @@ -1133,17 +1132,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 */ - 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. + * 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) - phv->phexpr = preprocess_phv_expression(root, phv->phexpr); + if (levelsup != 0) + IncrementVarSublevelsUp(node, -levelsup, 0); } else Assert(false); diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index c3c158a253d..85c24dbf09b 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -146,6 +146,7 @@ typedef struct /* Local functions */ static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind); +static void preprocess_subquery_phvs(PlannerInfo *root, Query *subquery); 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); @@ -1151,6 +1152,17 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, rte->subquery = (Query *) flatten_join_alias_vars(root, root->parse, (Node *) rte->subquery); + + /* + * Likewise, any PlaceHolderVars of our level that were pushed + * into the subquery have to be preprocessed now, so that they + * match the preprocessed PHVs of this level. Otherwise, a Var + * that preprocessing simplifies away at this level could remain + * in the subquery, and join removal could later delete that Var's + * rel. + */ + if (rte->lateral && root->glob->lastPHId != 0) + preprocess_subquery_phvs(root, rte->subquery); } else if (rte->rtekind == RTE_FUNCTION) { @@ -1645,20 +1657,43 @@ group_var_eqop(Query *parse, Var *var) } /* - * preprocess_phv_expression - * Do preprocessing on a PlaceHolderVar expression that's been pulled up. + * preprocess_subquery_phvs + * Preprocess the expressions of PlaceHolderVars of the current query + * level that appear within a LATERAL subquery. * * If a LATERAL subquery references an output of another subquery, and that * output must be wrapped in a PlaceHolderVar because of an intermediate outer - * join, then we'll push the PlaceHolderVar expression down into the subquery - * and later pull it back up during find_lateral_references, which runs after - * subquery_planner has preprocessed all the expressions that were in the - * current query level to start with. So we need to preprocess it then. + * join, then we'll push the PlaceHolderVar expression down into the subquery. + * Planning of the subquery won't preprocess it, since it belongs to our + * level, so we do that here. We modify the PHVs in place. This has to + * handle PHVs at any depth of sub-subqueries, so we temporarily adjust the + * expression's level to ours. */ -Expr * -preprocess_phv_expression(PlannerInfo *root, Expr *expr) +static void +preprocess_subquery_phvs(PlannerInfo *root, Query *subquery) { - return (Expr *) preprocess_expression(root, (Node *) expr, EXPRKIND_PHV); + List *vars = pull_vars_of_level((Node *) subquery, 1); + + foreach_ptr(Node, node, vars) + { + PlaceHolderVar *phv; + int levelsup; + Node *expr; + + if (!IsA(node, PlaceHolderVar)) + continue; + + phv = (PlaceHolderVar *) node; + levelsup = phv->phlevelsup; + + /* Adjust the expression to our level, preprocess it, and adjust back */ + expr = copyObject((Node *) phv->phexpr); + IncrementVarSublevelsUp(expr, -levelsup, 0); + expr = preprocess_expression(root, expr, EXPRKIND_PHV); + IncrementVarSublevelsUp(expr, levelsup, 0); + + phv->phexpr = (Expr *) expr; + } } /*-------------------- diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c index 222de1450b2..6a1be4551bd 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 907a255c36f..546480b625b 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -933,7 +933,15 @@ 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; + + /* + * A PHV of an upper query level can't contain join aliases of the + * target level, and its expression may have been preprocessed by that + * level already, so leave it alone. + */ + if (phv->phlevelsup > context->sublevels_up) + return node; /* no need to copy, really */ phv = (PlaceHolderVar *) expression_tree_mutator(node, flatten_join_alias_vars_mutator, diff --git a/src/include/optimizer/planner.h b/src/include/optimizer/planner.h index 9c4950b340f..d181c2f143c 100644 --- a/src/include/optimizer/planner.h +++ b/src/include/optimizer/planner.h @@ -77,7 +77,6 @@ extern void mark_partial_aggref(Aggref *agg, AggSplit aggsplit); extern Path *get_cheapest_fractional_path(RelOptInfo *rel, double tuple_fraction); -extern Expr *preprocess_phv_expression(PlannerInfo *root, Expr *expr); extern RelOptInfo *create_unique_paths(PlannerInfo *root, RelOptInfo *rel, SpecialJoinInfo *sjinfo); diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index db4fcc5a5a0..35898293b4c 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7043,6 +7043,106 @@ 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 + Replaces: Scan on i41 + 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) +(17 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 + Replaces: Scan on i41 + 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) +(20 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 expr_1)), (((SubPlan expr_1))) + -> Nested Loop Left Join + Output: ((SubPlan expr_1)) + Join Filter: false + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Result + Output: (SubPlan expr_1) + Replaces: Scan on i41 + One-Time Filter: false + SubPlan expr_1 + -> Result + Output: i41.f1 + -> Memoize + Output: (((SubPlan expr_1))) + Cache Key: ((SubPlan expr_1)) + Cache Mode: binary + -> Nested Loop + Output: ((SubPlan expr_1)) + -> Seq Scan on public.int4_tbl i43 + Output: i43.f1 + -> Materialize + -> Seq Scan on public.int4_tbl i44 +(24 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 9533af8656e..dc17b7d1d49 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2581,6 +2581,36 @@ 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; + -- More tests of correct placement of pseudoconstant quals -- simple constant-false condition -- 2.37.1 (Apple Git-137.1)