public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: Tom Lane <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Remove no-op PlaceHolderVars
Date: Fri, 16 Jan 2026 11:49:38 +0900
Message-ID: <CAMbWs4-_fDPOiYJWj330pJS++kKbTw_EfO5-pC8nPKHUAScMWw@mail.gmail.com> (raw)
In-Reply-To: <CAMbWs48vC37+j0V1SnHGRTP55AAaFyAYEbYGzh7UBKoWfh=E0w@mail.gmail.com>
References: <CAMbWs48biJp-vof82PNP_LzzFkURh0W+RKt4phoML-MyYavgdg@mail.gmail.com>
<[email protected]>
<CAMbWs48vC37+j0V1SnHGRTP55AAaFyAYEbYGzh7UBKoWfh=E0w@mail.gmail.com>
On Tue, Sep 3, 2024 at 5:50 PM Richard Guo <[email protected]> wrote:
> On Tue, Sep 3, 2024 at 11:31 AM Tom Lane <[email protected]> wrote:
> > Yeah. I've been mulling over how we could do this, and the real
> > problem is that the expression containing the PHV *has* been fully
> > preprocessed by the time we get to outer join strength reduction
> > (cf. file header comment in prepjointree.c). Simply dropping the PHV
> > can break various invariants that expression preprocessing is supposed
> > to establish, such as "no RelabelType directly above another" or "no
> > AND clause directly above another".
> Yeah, this is what the problem is.
While fixing some performance issues caused by PHVs recently, it
struck me again that we should be removing "no-op" PHVs whenever
possible, because PHVs can be optimization barriers in several cases.
I still do not have a good idea for ensuring that removing the PHV
wrapper does not break the expression tree invariants. But maybe we
can use a conservative approach: we only strip the PHV if the
contained expression is known to be safe (for example, a simple Var).
I've also realized that we cannot remove no-op PHVs in every case.
For example, deconstruct_distribute_oj_quals() relies on
remove_nulling_relids() to temporarily strip out the nullingrels bits
that are later restored as we crawl up the join stack. If the PHV
were removed, we would be unable to restore its nullingrels bits for
next level up. To handle this, I added a new parameter to
remove_nulling_relids() so the caller can decide whether to allow
removal or not.
Attached is a draft patch for the code changes. It currently causes
plan changes in 28 regression test queries, which is a surprisingly
high number. We'll have to go through these tests one by one, but
before doing that, I would like to hear others' thoughts on this
patch.
- Richard
Attachments:
[application/octet-stream] v2-0001-Remove-no-op-PlaceHolderVars.patch (19.8K, ../CAMbWs4-_fDPOiYJWj330pJS++kKbTw_EfO5-pC8nPKHUAScMWw@mail.gmail.com/2-v2-0001-Remove-no-op-PlaceHolderVars.patch)
download | inline diff:
From a41c27b140000a3b72f1b277dd21b2a3e93f5517 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Thu, 15 Jan 2026 11:43:17 +0900
Subject: [PATCH v2] Remove no-op PlaceHolderVars
When pulling up a subquery, we may need to wrap its targetlist items
in PlaceHolderVars if the subquery is under an outer join. This
ensures that they are evaluated at the right place and are forced to
NULL when the outer join should do so. However, if the outer join is
later reduced to an inner join, these PlaceHolderVars effectively
become no-ops regarding nullability.
It is desirable to remove these no-op PlaceHolderVars because they can
constrain optimization opportunities, such as blocking subexpression
folding or forcing join order. Previously, however, we did not try to
remove them because PHVs are also used to enforce separate identity of
subexpressions, and we could not distinguish between the two cases.
Additionally, simply removing the PHV wrapper could expose the
contained expression in a way that violates various invariants
established by expression preprocessing, such as creating nested AND
clauses or stacked RelabelType nodes.
This patch removes no-op PlaceHolderVars by marking PHVs as preserved
if they are used to enforce subexpression identity. Furthermore, to
ensure that stripping the PHV wrapper does not violate expression tree
invariants, we only do so when the contained expression is known to be
safe to expose to the surrounding expression structure.
---
src/backend/optimizer/path/equivclass.c | 4 +-
src/backend/optimizer/path/pathkeys.c | 2 +-
src/backend/optimizer/plan/initsplan.c | 6 +-
src/backend/optimizer/plan/planner.c | 4 +-
src/backend/optimizer/plan/setrefs.c | 4 +-
src/backend/optimizer/prep/prepjointree.c | 41 +++++++++++---
src/backend/optimizer/util/placeholder.c | 11 ++--
src/backend/optimizer/util/relnode.c | 4 +-
src/backend/optimizer/util/tlist.c | 2 +-
src/backend/rewrite/rewriteManip.c | 69 +++++++++++++++++++++--
src/backend/utils/adt/selfuncs.c | 8 ++-
src/include/nodes/pathnodes.h | 8 +++
src/include/rewrite/rewriteManip.h | 3 +-
13 files changed, 132 insertions(+), 34 deletions(-)
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index a4fcfcc86c8..d1dd6ba725f 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -2471,8 +2471,8 @@ reconsider_full_join_clause(PlannerInfo *root, OuterJoinClauseInfo *ojcinfo)
* representation for FULL JOIN USING output columns, this
* wouldn't be needed?)
*/
- cfirst = remove_nulling_relids(cfirst, fjrelids, NULL);
- csecond = remove_nulling_relids(csecond, fjrelids, NULL);
+ cfirst = remove_nulling_relids(cfirst, fjrelids, NULL, false);
+ csecond = remove_nulling_relids(csecond, fjrelids, NULL, false);
if (equal(leftvar, cfirst) && equal(rightvar, csecond))
{
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 5eb71635d15..0b610e46df8 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -1408,7 +1408,7 @@ make_pathkeys_for_sortclauses_extended(PlannerInfo *root,
sortkey = (Expr *)
remove_nulling_relids((Node *) sortkey,
bms_make_singleton(root->group_rtindex),
- NULL);
+ NULL, true);
}
pathkey = make_pathkey_from_sortop(root,
sortkey,
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index 97ea95a4eb8..071b39df0d4 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2643,13 +2643,15 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
*
* We first strip out all the nullingrels bits corresponding to
* commuting joins below this one, and then successively put them back
- * as we crawl up the join stack.
+ * as we crawl up the join stack. Note that we do not allow stripping
+ * no-op PlaceHolderVars here; if a PHV were removed, we would be
+ * unable to restore its nullingrels bits later.
*/
quals = jtitem->oj_joinclauses;
if (!bms_is_empty(joins_below))
quals = (List *) remove_nulling_relids((Node *) quals,
joins_below,
- NULL);
+ NULL, false);
/*
* We'll need to mark the lower versions of the quals as not safe to
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 7a6b8b749f2..9b897721de6 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5584,7 +5584,7 @@ make_group_input_target(PlannerInfo *root, PathTarget *final_target)
expr = (Expr *)
remove_nulling_relids((Node *) expr,
bms_make_singleton(root->group_rtindex),
- NULL);
+ NULL, true);
}
add_column_to_pathtarget(input_target, expr, sgref);
}
@@ -5628,7 +5628,7 @@ make_group_input_target(PlannerInfo *root, PathTarget *final_target)
non_group_vars = (List *)
remove_nulling_relids((Node *) non_group_vars,
bms_make_singleton(root->group_rtindex),
- NULL);
+ NULL, true);
}
add_new_columns_to_pathtarget(input_target, non_group_vars);
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 16d200cfb46..e63d52f0d8c 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -2523,11 +2523,11 @@ set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset)
plan->targetlist = (List *)
remove_nulling_relids((Node *) plan->targetlist,
bms_make_singleton(root->group_rtindex),
- NULL);
+ NULL, true);
plan->qual = (List *)
remove_nulling_relids((Node *) plan->qual,
bms_make_singleton(root->group_rtindex),
- NULL);
+ NULL, true);
}
output_targetlist = NIL;
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index c80bfc88d82..9e93775d91c 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -2725,8 +2725,9 @@ pullup_replace_vars_callback(Var *var,
* references to the same subquery output as being equal(). So it's worth
* a bit of extra effort to avoid it.
*
- * The cached items have phlevelsup = 0 and phnullingrels = NULL; we'll
- * copy them and adjust those values for this reference site below.
+ * The cached items have phlevelsup = 0, phnullingrels = NULL, and
+ * phpreserved = false. We'll copy them and adjust phpreserved here; the
+ * other values are adjusted for this reference site below.
*/
if (need_phv &&
varattno >= InvalidAttrNumber &&
@@ -2735,6 +2736,13 @@ pullup_replace_vars_callback(Var *var,
{
/* Just copy the entry and fall through to adjust phlevelsup etc */
newnode = copyObject(rcon->rv_cache[varattno]);
+
+ /*
+ * Mark the PlaceHolderVar as preserved if it's used for
+ * identification purposes.
+ */
+ if (rcon->wrap_option != REPLACE_WRAP_NONE)
+ ((PlaceHolderVar *) newnode)->phpreserved = true;
}
else
{
@@ -2920,6 +2928,13 @@ pullup_replace_vars_callback(Var *var,
if (varattno >= InvalidAttrNumber &&
varattno <= list_length(rcon->targetlist))
rcon->rv_cache[varattno] = copyObject(newnode);
+
+ /*
+ * Mark the PlaceHolderVar as preserved if it's used for
+ * identification purposes.
+ */
+ if (rcon->wrap_option != REPLACE_WRAP_NONE)
+ ((PlaceHolderVar *) newnode)->phpreserved = true;
}
}
}
@@ -3196,23 +3211,28 @@ reduce_outer_joins(PlannerInfo *root)
* remove references to those joins as nulling rels. This is handled as
* an additional pass, for simplicity and because we can handle all
* fully-reduced joins in a single pass over the parse tree.
+ *
+ * We allow stripping no-op PlaceHolderVars here, as this is a
+ * tree-simplification pass and we want to remove useless wrappers.
*/
if (!bms_is_empty(state2.inner_reduced))
{
root->parse = (Query *)
remove_nulling_relids((Node *) root->parse,
state2.inner_reduced,
- NULL);
+ NULL, true);
/* There could be references in the append_rel_list, too */
root->append_rel_list = (List *)
remove_nulling_relids((Node *) root->append_rel_list,
state2.inner_reduced,
- NULL);
+ NULL, true);
}
/*
* Partially-reduced full joins have to be done one at a time, since
* they'll each need a different setting of except_relids.
+ *
+ * As above, we allow stripping no-op PlaceHolderVars.
*/
foreach(lc, state2.partial_reduced)
{
@@ -3222,11 +3242,13 @@ reduce_outer_joins(PlannerInfo *root)
root->parse = (Query *)
remove_nulling_relids((Node *) root->parse,
full_join_relids,
- statep->unreduced_side);
+ statep->unreduced_side,
+ true);
root->append_rel_list = (List *)
remove_nulling_relids((Node *) root->append_rel_list,
full_join_relids,
- statep->unreduced_side);
+ statep->unreduced_side,
+ true);
}
}
@@ -3680,18 +3702,21 @@ remove_useless_result_rtes(PlannerInfo *root)
* know that such an outer join wouldn't really have nulled anything.) We
* don't do this during the main recursion, for simplicity and because we
* can handle all such joins in a single pass over the parse tree.
+ *
+ * We allow stripping no-op PlaceHolderVars here, as this is a
+ * tree-simplification pass and we want to remove useless wrappers.
*/
if (!bms_is_empty(dropped_outer_joins))
{
root->parse = (Query *)
remove_nulling_relids((Node *) root->parse,
dropped_outer_joins,
- NULL);
+ NULL, true);
/* There could be references in the append_rel_list, too */
root->append_rel_list = (List *)
remove_nulling_relids((Node *) root->append_rel_list,
dropped_outer_joins,
- NULL);
+ NULL, true);
}
/*
diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
index e1706363c88..4279fd85117 100644
--- a/src/backend/optimizer/util/placeholder.c
+++ b/src/backend/optimizer/util/placeholder.c
@@ -44,11 +44,11 @@ static bool contain_placeholder_references_walker(Node *node,
* phrels is the syntactic location (as a set of relids) to attribute
* to the expression.
*
- * The caller is responsible for adjusting phlevelsup and phnullingrels
- * as needed. Because we do not know here which query level the PHV
- * will be associated with, it's important that this function touches
- * only root->glob; messing with other parts of PlannerInfo would be
- * likely to do the wrong thing.
+ * The caller is responsible for adjusting phlevelsup, phnullingrels, and
+ * phpreserved as needed. Because we do not know here which query level
+ * the PHV will be associated with, it's important that this function
+ * touches only root->glob; messing with other parts of PlannerInfo
+ * would be likely to do the wrong thing.
*/
PlaceHolderVar *
make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels)
@@ -58,6 +58,7 @@ make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels)
phv->phexpr = expr;
phv->phrels = phrels;
phv->phnullingrels = NULL; /* caller may change this later */
+ phv->phpreserved = false; /* caller may change this later */
phv->phid = ++(root->glob->lastPHId);
phv->phlevelsup = 0; /* caller may change this later */
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index f57631e876f..2d4574ce916 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -2264,11 +2264,11 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (bms_overlap(rel1->relids, root->outer_join_rels))
expr1 = (Expr *) remove_nulling_relids((Node *) expr1,
root->outer_join_rels,
- NULL);
+ NULL, false);
if (bms_overlap(rel2->relids, root->outer_join_rels))
expr2 = (Expr *) remove_nulling_relids((Node *) expr2,
root->outer_join_rels,
- NULL);
+ NULL, false);
}
/*
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 752ea9222f6..ca66b07009e 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -1161,7 +1161,7 @@ split_pathtarget_walker(Node *node, split_pathtarget_context *context)
sanitized_node =
remove_nulling_relids(node,
bms_make_singleton(context->root->group_rtindex),
- NULL);
+ NULL, true);
}
/*
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 6fa174412f2..16c295b2c14 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -53,6 +53,7 @@ typedef struct
const Bitmapset *removable_relids;
const Bitmapset *except_relids;
int sublevels_up;
+ bool remove_noop_phvs;
} remove_nulling_relids_context;
static bool contain_aggs_of_level_walker(Node *node,
@@ -68,6 +69,7 @@ static Node *add_nulling_relids_mutator(Node *node,
add_nulling_relids_context *context);
static Node *remove_nulling_relids_mutator(Node *node,
remove_nulling_relids_context *context);
+static bool is_safe_to_strip_payload(Node *node);
/*
@@ -1323,17 +1325,22 @@ add_nulling_relids_mutator(Node *node,
* in Var.varnullingrels and PlaceHolderVar.phnullingrels fields within
* the given expression, except in nodes belonging to rels listed in
* except_relids.
+ *
+ * If remove_noop_phvs is true, we also check if PlaceHolderVars become
+ * no-ops and strip them if so.
*/
Node *
remove_nulling_relids(Node *node,
const Bitmapset *removable_relids,
- const Bitmapset *except_relids)
+ const Bitmapset *except_relids,
+ bool remove_noop_phvs)
{
remove_nulling_relids_context context;
context.removable_relids = removable_relids;
context.except_relids = except_relids;
context.sublevels_up = 0;
+ context.remove_noop_phvs = remove_noop_phvs;
return query_or_expression_tree_mutator(node,
remove_nulling_relids_mutator,
&context,
@@ -1371,11 +1378,15 @@ remove_nulling_relids_mutator(Node *node,
!bms_overlap(phv->phrels, context->except_relids))
{
/*
- * Note: it might seem desirable to remove the PHV altogether if
- * phnullingrels goes to empty. Currently we dare not do that
- * because we use PHVs in some cases to enforce separate identity
- * of subexpressions; see wrap_option usages in prepjointree.c.
+ * If phnullingrels goes to empty, the PHV is no longer needed for
+ * outer-join nulling. We can remove it if we are allowed to
+ * remove no-op PHVs, provided that it is not marked as preserved
+ * (which indicates that it is needed to enforce separate identity
+ * of subexpressions) and that the contained expression is safe to
+ * pull up without breaking various invariants established by
+ * expression preprocessing.
*/
+
/* Copy the PlaceHolderVar and mutate what's below ... */
phv = (PlaceHolderVar *)
expression_tree_mutator(node,
@@ -1388,6 +1399,14 @@ remove_nulling_relids_mutator(Node *node,
phv->phrels = bms_difference(phv->phrels,
context->removable_relids);
Assert(!bms_is_empty(phv->phrels));
+
+ /* Strip the PHV if it's safe; see comment above */
+ if (context->remove_noop_phvs &&
+ !phv->phpreserved &&
+ bms_is_empty(phv->phnullingrels) &&
+ is_safe_to_strip_payload((Node *) phv->phexpr))
+ return (Node *) phv->phexpr;
+
return (Node *) phv;
}
/* Otherwise fall through to copy the PlaceHolderVar normally */
@@ -1408,6 +1427,46 @@ remove_nulling_relids_mutator(Node *node,
return expression_tree_mutator(node, remove_nulling_relids_mutator, context);
}
+/*
+ * is_safe_to_strip_payload
+ *
+ * Check whether the given node is safe to pull up into the surrounding
+ * expression structure.
+ *
+ * This is used to remove a PlaceHolderVar. Even if a PlaceHolderVar is no
+ * longer needed, we cannot simply remove it if doing so would expose the
+ * contained expression to a parent node in a way that breaks various
+ * invariants established by earlier expression preprocessing.
+ *
+ * Since we don't know what the parent node is, we need to be conservative.
+ */
+static bool
+is_safe_to_strip_payload(Node *node)
+{
+ if (node == NULL)
+ return false;
+
+ switch (nodeTag(node))
+ {
+ case T_Var:
+ case T_Param:
+ case T_Const:
+ case T_PlaceHolderVar:
+ /* Atomic nodes should be safe to expose */
+ return true;
+
+ case T_OpExpr:
+ case T_FuncExpr:
+ case T_CoalesceExpr:
+ return true;
+
+ default:
+ return false;
+ }
+
+ return false;
+}
+
/*
* replace_rte_variables() finds all Vars in an expression tree
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 29fec655593..fdf328c0fac 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3655,7 +3655,7 @@ add_unique_group_var(PlannerInfo *root, List *varinfos,
* extended statistics (see estimate_multivariate_ndistinct). So strip
* them out first.
*/
- var = remove_nulling_relids(var, root->outer_join_rels, NULL);
+ var = remove_nulling_relids(var, root->outer_join_rels, NULL, false);
foreach(lc, varinfos)
{
@@ -4236,7 +4236,8 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
* Clear nullingrels to correctly match hash keys. See
* add_unique_group_var()'s comment for details.
*/
- expr = remove_nulling_relids(expr, root->outer_join_rels, NULL);
+ expr = remove_nulling_relids(expr, root->outer_join_rels,
+ NULL, false);
/*
* Detect and exclude exact duplicates from the list of hash
@@ -5762,7 +5763,8 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
* extended statistics. So strip them out first.
*/
if (bms_overlap(varnos, root->outer_join_rels))
- node = remove_nulling_relids(node, root->outer_join_rels, NULL);
+ node = remove_nulling_relids(node, root->outer_join_rels,
+ NULL, false);
foreach(ilist, onerel->indexlist)
{
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 449885b9319..c4671ef362a 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -2985,6 +2985,11 @@ typedef struct MergeScanSelCache
* level of a PlaceHolderVar might be a join rather than a base relation.
* Likewise, phnullingrels corresponds to varnullingrels.
*
+ * phpreserved indicates whether the PlaceHolderVar needs to be preserved
+ * when its phnullingrels becomes empty. This is set true in cases where
+ * the PlaceHolderVar is used to enforce the separate identity of the
+ * contained expression.
+ *
* Although the planner treats this as an expression node type, it is not
* recognized by the parser or executor, so we declare it here rather than
* in primnodes.h.
@@ -3018,6 +3023,9 @@ typedef struct PlaceHolderVar
/* RT indexes of outer joins that can null PHV's value */
Relids phnullingrels;
+ /* true if PHV enforces separate identity */
+ bool phpreserved pg_node_attr(equal_ignore);
+
/* ID for PHV (unique within planner run) */
Index phid;
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
index f8216c22fb7..05245faa49c 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -89,7 +89,8 @@ extern Node *add_nulling_relids(Node *node,
const Bitmapset *added_relids);
extern Node *remove_nulling_relids(Node *node,
const Bitmapset *removable_relids,
- const Bitmapset *except_relids);
+ const Bitmapset *except_relids,
+ bool remove_noop_phvs);
extern Node *replace_rte_variables(Node *node,
int target_varno, int sublevels_up,
--
2.39.5 (Apple Git-154)
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Remove no-op PlaceHolderVars
In-Reply-To: <CAMbWs4-_fDPOiYJWj330pJS++kKbTw_EfO5-pC8nPKHUAScMWw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox