From 8ea97ee348d4de725dc73df987f7dd28d755efe2 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Sat, 19 Sep 2026 21:57:46 +0900 Subject: [PATCH] Fix preprocessing of PHV copies pushed down into subqueries Commit e28cf96e9 preprocessed the copies of a query level's PlaceHolderVars that were pushed down into its subqueries in a single walk at the start of subquery_planner. That was wrong in two ways. First, such copies can also be inserted later, when a join alias Var of the outer level is expanded within a LATERAL subquery or a SubLink's subselect and the alias expression contains a PHV. Those copies were never preprocessed, so a SubLink within them survived into the subquery's lateral references or into a SubPlan's argument list, tripping an assertion in identify_current_nestloop_params or failing in cost_qual_eval. Second, when a pushed-down copy directly contains another PHV of the same level, the walk preprocessed the inner one first and then the outer one, so the outer one's preprocessing ran into the SubPlans just created in the inner one and tripped an assertion in flatten_join_alias_vars. To fix, preprocess the copies right after join alias expansion instead: within preprocess_expression, just before SubLinks are turned into SubPlans, and for LATERAL subquery RTEs, right after their aliases are flattened. Preprocessing a copy handles everything within it, including any further copies nested inside its SubLinks, so the walker no longer looks inside a copy. Back-patch to v16, as with commit e28cf96e9. Reported-by: Fujii Masao Author: Richard Guo Discussion: https://postgr.es/m/CAHGQGwH-acRnAk1=Ki4EiAJEt67+Rz=_jyg7pbP_c6BK4oHZ4Q@mail.gmail.com Backpatch-through: 16 --- src/backend/optimizer/plan/planner.c | 50 +++++---- src/backend/optimizer/plan/subselect.c | 7 +- src/backend/optimizer/util/var.c | 9 +- src/test/regress/expected/join.out | 142 +++++++++++++++++++++++-- src/test/regress/sql/join.sql | 35 ++++++ 5 files changed, 209 insertions(+), 34 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 18424f4b81d..4aef57aae58 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -138,10 +138,10 @@ typedef struct /* Local functions */ static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind); -static void preprocess_subquery_phvs(PlannerInfo *root); +static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode); +static void preprocess_subquery_phvs(PlannerInfo *root, Node *node); 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); static grouping_sets_data *preprocess_grouping_sets(PlannerInfo *root); static List *remap_to_groupclause_idx(List *groupClause, List *gsets, @@ -830,14 +830,6 @@ subquery_planner(PlannerGlobal *glob, Query *parse, */ 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 @@ -959,6 +951,14 @@ subquery_planner(PlannerGlobal *glob, Query *parse, rte->subquery = (Query *) flatten_join_alias_vars(root, root->parse, (Node *) rte->subquery); + + /* + * Likewise for copies of our PlaceHolderVars in the subquery. + * This must be done after the alias expansion above, which can + * insert such copies. + */ + if (rte->lateral && root->glob->lastPHId != 0) + preprocess_subquery_phvs(root, (Node *) rte->subquery); } else if (rte->rtekind == RTE_FUNCTION) { @@ -1203,6 +1203,15 @@ preprocess_expression(PlannerInfo *root, Node *expr, int kind) convert_saop_to_hashed_saop(expr); } + /* + * Preprocess any copies of our PlaceHolderVars within SubLink subselects. + * This must be done after join alias expansion, which can insert such + * copies, and before the SubLinks are turned into SubPlans, which collect + * those copies as SubPlan arguments. + */ + if (root->parse->hasSubLinks && root->glob->lastPHId != 0) + preprocess_subquery_phvs(root, expr); + /* Expand SubLinks to SubPlans */ if (root->parse->hasSubLinks) expr = SS_process_sublinks(root, expr, (kind == EXPRKIND_QUAL)); @@ -1286,24 +1295,25 @@ preprocess_phv_expression(PlannerInfo *root, Expr *expr) /* * preprocess_subquery_phvs * Preprocess copies of this level's PlaceHolderVars that were pushed - * down into subqueries. + * down into subqueries within the given tree. * * 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. + * our level. Preprocessing a copy's expression takes care of everything + * within it, including any further copies nested inside SubLinks there, so + * we don't look inside a copy ourselves. */ static void -preprocess_subquery_phvs(PlannerInfo *root) +preprocess_subquery_phvs(PlannerInfo *root, Node *node) { preprocess_subquery_phvs_context context; context.root = root; context.sublevels_up = 0; - (void) query_tree_walker(root->parse, preprocess_subquery_phvs_walker, - &context, 0); + (void) preprocess_subquery_phvs_walker(node, &context); } static bool @@ -1327,9 +1337,9 @@ preprocess_subquery_phvs_walker(Node *node, { PlaceHolderVar *phv = (PlaceHolderVar *) node; - /* Handle any nested copies within the expression first */ - (void) expression_tree_walker(node, preprocess_subquery_phvs_walker, - context); + /* A PHV of an upper level can't contain anything of our level */ + if (phv->phlevelsup > context->sublevels_up) + return false; /* * Is this a copy of one of our PHVs that is pushed down into a @@ -1347,8 +1357,10 @@ preprocess_subquery_phvs_walker(Node *node, expr = preprocess_expression(context->root, expr, EXPRKIND_PHV); IncrementVarSublevelsUp(expr, levelsup, 0); phv->phexpr = (Expr *) expr; + return false; } - return false; + + /* Otherwise, it's ours or a lower level's; look inside it */ } return expression_tree_walker(node, preprocess_subquery_phvs_walker, context); diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 3c40362e483..d4fdcab5c0b 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -1962,9 +1962,10 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context) /* * Don't recurse into the arguments of an outer PHV, Aggref or * GroupingFunc here. Any SubLinks in the arguments have to be dealt with - * at the outer query level; they'll be handled when build_subplan - * collects the PHV, Aggref or GroupingFunc into the arguments to be - * passed down to the current subplan. + * at the outer query level; for an Aggref or GroupingFunc, they'll be + * handled when build_subplan collects it into the arguments to be passed + * down to the current subplan, while an outer PHV's expression has + * already been preprocessed by its owning level. */ if (IsA(node, PlaceHolderVar)) { diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 110d8c7f8db..6600ccb9eca 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -868,11 +868,10 @@ flatten_join_alias_vars_mutator(Node *node, 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. + * A pushed-down copy of a target-level PHV is preprocessed by + * preprocess_subquery_phvs, which handles its target-level aliases + * and may leave SubPlans in it; so don't recurse into it. Any other + * PHV may still hold target-level aliases, so recurse. */ if (phv->phlevelsup == context->sublevels_up && context->sublevels_up > 0) diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 47f48b38f7e..37ac979e74e 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -5854,24 +5854,24 @@ from (select case when false then remov.id else (select i41.f1) end as c1 QUERY PLAN --------------------------------------------------------- Nested Loop - Output: ((SubPlan 2)), (((SubPlan 2))) + Output: ((SubPlan 1)), (((SubPlan 1))) -> Nested Loop Left Join - Output: ((SubPlan 2)) + Output: ((SubPlan 1)) Join Filter: false -> Seq Scan on public.int4_tbl i42 Output: i42.f1 -> Result - Output: (SubPlan 2) + Output: (SubPlan 1) One-Time Filter: false - SubPlan 2 + SubPlan 1 -> Result Output: f1 -> Memoize - Output: (((SubPlan 2))) - Cache Key: ((SubPlan 2)) + Output: (((SubPlan 1))) + Cache Key: ((SubPlan 1)) Cache Mode: binary -> Nested Loop - Output: ((SubPlan 2)) + Output: ((SubPlan 1)) -> Seq Scan on public.int4_tbl i43 Output: i43.f1 -> Materialize @@ -5926,6 +5926,134 @@ from (select case when false then remov.id else (select i41.f1) end as c1 Output: ((SubPlan 1)) (14 rows) +-- likewise, where the pushed-down PHV's expression contains another PHV of +-- the same level, which must not be preprocessed separately from its parent +explain (verbose, costs off) +select ss3.c2 +from int4_tbl i41 + left join (select coalesce(ss1.c1, 0) as c1 + from int4_tbl i42 + left join (select (select i43.f1) as c1 + from int4_tbl i43) ss1 on true) ss2 + on true, + lateral (select ss2.c1 as c2 from int4_tbl i44 offset 0) ss3; + QUERY PLAN +--------------------------------------------------------------- + Nested Loop + Output: ((COALESCE(((SubPlan 1)), 0))) + -> Nested Loop Left Join + Output: (COALESCE(((SubPlan 1)), 0)) + -> Seq Scan on public.int4_tbl i41 + Output: i41.f1 + -> Materialize + Output: (COALESCE(((SubPlan 1)), 0)) + -> Nested Loop Left Join + Output: COALESCE(((SubPlan 1)), 0) + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Materialize + Output: ((SubPlan 1)) + -> Seq Scan on public.int4_tbl i43 + Output: (SubPlan 1) + SubPlan 1 + -> Result + Output: i43.f1 + -> Memoize + Output: ((COALESCE(((SubPlan 1)), 0))) + Cache Key: (COALESCE(((SubPlan 1)), 0)) + Cache Mode: binary + -> Seq Scan on public.int4_tbl i44 + Output: (COALESCE(((SubPlan 1)), 0)) +(25 rows) + +-- likewise, where the PHV copy is only inserted into the LATERAL subquery by +-- expanding a join alias Var of the outer level +explain (verbose, costs off) +select ss2.c2 +from ((select (select i41.f1) as c1 from int4_tbl i41) ss1 + full join int4_tbl i42(c1) using (c1)) j, + lateral (select j.c1 as c2 from int4_tbl i43 offset 0) ss2; + QUERY PLAN +--------------------------------------------------- + Nested Loop + Output: (COALESCE(((SubPlan 1)), i42.c1)) + -> Hash Full Join + Output: ((SubPlan 1)), i42.c1 + Hash Cond: (((SubPlan 1)) = i42.c1) + -> Seq Scan on public.int4_tbl i41 + Output: (SubPlan 1) + SubPlan 1 + -> Result + Output: i41.f1 + -> Hash + Output: i42.c1 + -> Seq Scan on public.int4_tbl i42 + Output: i42.c1 + -> Seq Scan on public.int4_tbl i43 + Output: COALESCE(((SubPlan 1)), i42.c1) +(16 rows) + +-- likewise, where the join alias is expanded within a SubLink's subselect +explain (verbose, costs off) +select (select j.c1 from int4_tbl i43 offset 0) as c2 +from ((select (select i41.f1) as c1 from int4_tbl i41) ss1 + full join int4_tbl i42(c1) using (c1)) j; + QUERY PLAN +--------------------------------------------------- + Hash Full Join + Output: (SubPlan 2) + Hash Cond: (((SubPlan 1)) = i42.c1) + -> Seq Scan on public.int4_tbl i41 + Output: (SubPlan 1) + SubPlan 1 + -> Result + Output: i41.f1 + -> Hash + Output: i42.c1 + -> Seq Scan on public.int4_tbl i42 + Output: i42.c1 + SubPlan 2 + -> Seq Scan on public.int4_tbl i43 + Output: COALESCE(((SubPlan 1)), i42.c1) +(15 rows) + +-- likewise, where the SubLink holding the copy ends up in the translated +-- Vars of an appendrel child pulled up from a LATERAL UNION ALL subquery +explain (verbose, costs off) +select ss2.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, + lateral ((select (select ss1.c1) as c2) union all (select (select ss1.c1))) ss2; + QUERY PLAN +----------------------------------------------------- + Nested Loop + Output: ((SubPlan 1)) + -> Nested Loop Left Join + Output: (NULL::integer) + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Materialize + Output: (NULL::integer) + -> Seq Scan on public.int4_tbl i41 + Output: NULL::integer + -> Memoize + Output: ((SubPlan 1)) + Cache Key: (NULL::integer), (NULL::integer) + Cache Mode: binary + -> Append + -> Result + Output: (SubPlan 1) + SubPlan 1 + -> Result + Output: (NULL::integer) + -> Result + Output: (SubPlan 2) + SubPlan 2 + -> Result + Output: (NULL::integer) +(25 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 2171a8f2bd6..fc984fff672 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2164,6 +2164,41 @@ 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; +-- likewise, where the pushed-down PHV's expression contains another PHV of +-- the same level, which must not be preprocessed separately from its parent +explain (verbose, costs off) +select ss3.c2 +from int4_tbl i41 + left join (select coalesce(ss1.c1, 0) as c1 + from int4_tbl i42 + left join (select (select i43.f1) as c1 + from int4_tbl i43) ss1 on true) ss2 + on true, + lateral (select ss2.c1 as c2 from int4_tbl i44 offset 0) ss3; + +-- likewise, where the PHV copy is only inserted into the LATERAL subquery by +-- expanding a join alias Var of the outer level +explain (verbose, costs off) +select ss2.c2 +from ((select (select i41.f1) as c1 from int4_tbl i41) ss1 + full join int4_tbl i42(c1) using (c1)) j, + lateral (select j.c1 as c2 from int4_tbl i43 offset 0) ss2; + +-- likewise, where the join alias is expanded within a SubLink's subselect +explain (verbose, costs off) +select (select j.c1 from int4_tbl i43 offset 0) as c2 +from ((select (select i41.f1) as c1 from int4_tbl i41) ss1 + full join int4_tbl i42(c1) using (c1)) j; + +-- likewise, where the SubLink holding the copy ends up in the translated +-- Vars of an appendrel child pulled up from a LATERAL UNION ALL subquery +explain (verbose, costs off) +select ss2.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, + lateral ((select (select ss1.c1) as c2) union all (select (select ss1.c1))) ss2; + -- More tests of correct placement of pseudoconstant quals -- simple constant-false condition