From 8a2d357a12118098f503ea0cbcf839d22ed670b4 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Thu, 1 Aug 2024 11:08:14 +0900 Subject: [PATCH v12 3/3] Unwrap a PlaceHolderVar when safe It's desirable to remove a PlaceHolderVar altogether and use its contained expression instead when its phnullingrels goes to empty. Currently we dare not do that because we use PHVs in some cases to enforce separate identity of subexpressions. But in cases where the PHV is used to carry the nullingrel bit of the RTE_GROUP RT index, we know we can do that. This patch includes a flag to indicate whether it's safe to unwrap a PlaceHolderVar, and set this flag to true for the PHVs created in mark_nullable_by_grouping. --- src/backend/optimizer/util/placeholder.c | 11 ++++++----- src/backend/optimizer/util/var.c | 2 ++ src/backend/rewrite/rewriteManip.c | 15 ++++++++++++--- src/include/nodes/pathnodes.h | 10 ++++++++++ src/test/regress/expected/groupingsets.out | 19 ++++++++----------- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c index 81abadd6db..5a366ea952 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 remove_safe 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) @@ -60,6 +60,7 @@ make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels) phv->phnullingrels = NULL; /* caller may change this later */ phv->phid = ++(root->glob->lastPHId); phv->phlevelsup = 0; /* caller may change this later */ + phv->remove_safe = false; /* caller may change this later */ return phv; } diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index a3e3037f26..0cea08b9c6 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -1129,6 +1129,8 @@ mark_nullable_by_grouping(PlannerInfo *root, Node *newnode, Var *oldvar) /* newphv has zero phlevelsup and NULL phnullingrels; fix it */ newphv->phlevelsup = oldvar->varlevelsup; newphv->phnullingrels = bms_copy(oldvar->varnullingrels); + /* It's safe to be removed when phnullingrels becomes empty */ + newphv->remove_safe = true; newnode = (Node *) newphv; } } diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index 191f2dc0b1..b2c4c9ca50 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -1278,9 +1278,10 @@ remove_nulling_relids_mutator(Node *node, { /* * 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_non_vars usages in prepjointree.c. + * phnullingrels goes to empty. Currently we only dare to do that + * if the PHV is marked remove_safe, because we use PHVs in some + * cases to enforce separate identity of subexpressions; see + * wrap_non_vars usages in prepjointree.c. */ /* Copy the PlaceHolderVar and mutate what's below ... */ phv = (PlaceHolderVar *) @@ -1294,6 +1295,14 @@ remove_nulling_relids_mutator(Node *node, phv->phrels = bms_difference(phv->phrels, context->removable_relids); Assert(!bms_is_empty(phv->phrels)); + + /* + * Remove the PHV altogether if it's marked remove_safe and + * phnullingrels goes to empty. + */ + if (phv->remove_safe && bms_is_empty(phv->phnullingrels)) + return (Node *) phv->phexpr; + return (Node *) phv; } /* Otherwise fall through to copy the PlaceHolderVar normally */ diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index e48cb10b89..62a531c25c 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -2763,6 +2763,13 @@ typedef struct MergeScanSelCache * level of a PlaceHolderVar might be a join rather than a base relation. * Likewise, phnullingrels corresponds to varnullingrels. * + * remove_safe is true if it is safe to remove the PHV and use its contained + * expression instead when phnullingrels becomes empty. This is set true in + * cases where the PHV is used to carry the nullingrel bit of the RTE_GROUP RT + * index. In other cases we do not set this flag because PHVs might be used to + * enforce separate identity of subexpressions; see wrap_non_vars usages in + * prepjointree.c. + * * 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. @@ -2801,6 +2808,9 @@ typedef struct PlaceHolderVar /* > 0 if PHV belongs to outer query */ Index phlevelsup; + + /* true if PHV is safe to be removed when phnullingrels becomes empty */ + bool remove_safe; } PlaceHolderVar; /* diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out index fdfd8251e0..d83a0ccaed 100644 --- a/src/test/regress/expected/groupingsets.out +++ b/src/test/regress/expected/groupingsets.out @@ -442,22 +442,19 @@ select * from ( group by grouping sets(1, 2) ) ss where x = 1 and q1 = 123; - QUERY PLAN --------------------------------------------------- + QUERY PLAN +-------------------------------------------- Subquery Scan on ss Output: ss.x, ss.q1, ss.sum Filter: ((ss.x = 1) AND (ss.q1 = 123)) -> GroupAggregate Output: (1), i1.q1, sum(i1.q2) - Group Key: (1) + Group Key: 1 Sort Key: i1.q1 Group Key: i1.q1 - -> Sort - Output: (1), i1.q1, i1.q2 - Sort Key: (1) - -> Seq Scan on public.int8_tbl i1 - Output: 1, i1.q1, i1.q2 -(13 rows) + -> Seq Scan on public.int8_tbl i1 + Output: 1, i1.q1, i1.q2 +(10 rows) select * from ( select 1 as x, q1, sum(q2) @@ -2354,8 +2351,8 @@ select 1 as one group by rollup(one) order by one nulls first; ----------------------------- Sort Sort Key: (1) NULLS FIRST - -> MixedAggregate - Hash Key: 1 + -> GroupAggregate + Group Key: 1 Group Key: () -> Result (6 rows) -- 2.43.0