public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: Dean Rasheed <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: jian he <[email protected]>
Cc: Zhang Mingli <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Virtual generated columns
Date: Sat, 22 Feb 2025 23:55:41 +0900
Message-ID: <CAMbWs4_DVtFfonq7eL6Ocor4apV9dBb6wOhJ5iUZPEddKc-_vg@mail.gmail.com> (raw)
In-Reply-To: <CAEZATCV+msUomqYUc-M70epBn7WppLqiw1z=4u4yf6w4vUECiQ@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CAEZATCXnp5XTkUCsmfjLXVWZ7TaibWatnXDdu52J6zN3XsyZhw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAEZATCX6SHevKrVfAOq+88b4Ysm4Rqq2yeO6He3N0pttEbjYag@mail.gmail.com>
<[email protected]>
<CAEZATCVsfdp5MQpENpbrFAAfpC2tYU7y-Q=uOG57UubGy48Gyw@mail.gmail.com>
<[email protected]>
<[email protected]>
<8d6a3d4c-f630-4cf9-aced-6cead95776e5@Spark>
<CAMbWs4-h49s0ehfSq4CnztRLSwOF79ZQUkSPzGCQLNroAG7joA@mail.gmail.com>
<CACJufxHVsCoxhqeDV973Hny3EtgCGztDJj=KKvErexnH=2k6yQ@mail.gmail.com>
<12d5cce9-917f-4a99-86d3-715bb3b363bb@Spark>
<CAMbWs4_xiGiB5Oufdn62cJ=MoiS9cJWLzq=jR_7xnTO8x_oPqg@mail.gmail.com>
<CACJufxEWnz__Bf-KhVaiisHZd-5JcRRY_xttCKLs_m2xaUJmQA@mail.gmail.com>
<[email protected]>
<CAEZATCUpzF0XZGVpuJ-O4WLnDWBmddSc+yMWvg6zsz_ekegfPg@mail.gmail.com>
<CAMbWs4-JmGEBDucPw8iJ3H8hg-7TkbOqE0WTVzhJ5iyzQyP=-A@mail.gmail.com>
<CAMbWs4_LRNGAxbQbLOrj+VMy80dUR5v2JQ9fVcre10C4KVYUDg@mail.gmail.com>
<CAEZATCWhr=FM4X5kCPvVs-g2XEk+ceLsNtBK_zZMkqFn9vUjsw@mail.gmail.com>
<CAEZATCWsKqCtZ=ud26-gGV3zHt-hjS4OKG43GCBhSaYUWyfKiw@mail.gmail.com>
<CAMbWs4_Rg-F=jQ7-WfBztnVZG2i5kAUwoQv0O5GbGPzxpjDeTQ@mail.gmail.com>
<CAEZATCV+msUomqYUc-M70epBn7WppLqiw1z=4u4yf6w4vUECiQ@mail.gmail.com>
On Sat, Feb 22, 2025 at 2:35 AM Dean Rasheed <[email protected]> wrote:
> On Fri, 21 Feb 2025 at 06:16, Richard Guo <[email protected]> wrote:
> > * The expansion of virtual generated columns occurs after subquery
> > pullup, which can lead to issues. This was an oversight on my part.
> > Initially, I believed it wasn't possible for an RTE_RELATION RTE to
> > have 'lateral' set to true, so I assumed it would be safe to expand
> > virtual generated columns after subquery pullup. However, upon closer
> > look, this doesn't seem to be the case: if a subquery had a LATERAL
> > marker, that would be propagated to any of its child RTEs, even for
> > RTE_RELATION child RTE if this child rel has sampling info (see
> > pull_up_simple_subquery).
>
> Ah yes. That matches my initial instinct, which was to expand virtual
> generated columns early in the planning process, but I didn't properly
> understand why that was necessary.
After chewing on this point for a bit longer, I think the virtual
generated columns should be expanded after we have pulled up any
SubLinks within the query's quals; otherwise any virtual generated
column references within the SubLinks that should be transformed into
joins wouldn't get expanded. As an example, please consider:
create table t (a int, b int);
create table vt (a int, b int generated always as (a * 2));
insert into t values (1, 1);
insert into vt values (1);
# select 1 from t t1 where exists
(select 1 from vt where exists
(select t1.a from t t2 where vt.b = 2));
ERROR: unexpected virtual generated column reference
> LGTM aside from a comment in fireRIRrules() that needed updating and a
> minor issue in the callback function: when deciding whether to wrap
> newnode in a ReturningExpr, if newnode is a Var, it should now compare
> its varlevelsup with 0, not var->varlevelsup, since newnode hasn't had
> its varlevelsup adjusted at that point.
Nice catch.
Attached are the updated patches to fix all the mentioned issues. I
plan to push them early next week after staring at the code for a bit
longer, barring any objections.
Thanks
Richard
Attachments:
[application/octet-stream] v7-0001-Expand-virtual-generated-columns-in-the-planner.patch (22.9K, ../CAMbWs4_DVtFfonq7eL6Ocor4apV9dBb6wOhJ5iUZPEddKc-_vg@mail.gmail.com/2-v7-0001-Expand-virtual-generated-columns-in-the-planner.patch)
download | inline diff:
From c9820a83194a5b6e76fc56f17352e0630a5f506f Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Fri, 21 Feb 2025 09:46:59 +0900
Subject: [PATCH v7 1/2] Expand virtual generated columns in the planner
Commit 83ea6c540 added support for virtual generated columns, which
were expanded in the rewriter, in a similar way to view columns.
However, this approach has several issues. If a Var referencing a
virtual generated column has any varnullingrels, there would be no way
to propagate the varnullingrels into the generation expression,
leading to "wrong varnullingrels" errors and improper outer-join
removal. Additionally, if such a Var comes from the nullable side of
an outer join, we may need to wrap the generation expression in a
PlaceHolderVar to ensure that it is evaluated at the right place and
hence is forced to null when the outer join should do so. In some
cases, such as when the query uses grouping sets, we also need a
PlaceHolderVar for anything that's not a simple Var to isolate
subexpressions. All of this cannot be achieved in the rewriter.
To fix this, the patch expands the virtual generated columns in the
planner and leverages the pullup_replace_vars architecture to avoid
code duplication. This requires handling the OLD/NEW RETURNING list
Vars in pullup_replace_vars_callback.
---
src/backend/optimizer/plan/planner.c | 8 +
src/backend/optimizer/prep/prepjointree.c | 191 ++++++++++++++++++
src/backend/rewrite/rewriteHandler.c | 91 ++++-----
src/backend/rewrite/rewriteManip.c | 2 +-
src/include/optimizer/prep.h | 1 +
src/include/rewrite/rewriteHandler.h | 1 +
src/include/rewrite/rewriteManip.h | 3 +
.../regress/expected/generated_virtual.out | 89 ++++++++
src/test/regress/sql/generated_virtual.sql | 38 ++++
9 files changed, 378 insertions(+), 46 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 7b1a8a0a9f1..36ee6dd43de 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -734,6 +734,14 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
*/
preprocess_function_rtes(root);
+ /*
+ * Scan the rangetable for relations with virtual generated columns, and
+ * replace all Var nodes in the query that reference these columns with
+ * the generation expressions. Recursion issues here are handled in the
+ * same way as for SubLinks.
+ */
+ parse = root->parse = expand_virtual_generated_columns(root);
+
/*
* Check to see if any subqueries in the jointree can be merged into this
* query.
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 5d9225e9909..585b4e5f2de 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -7,6 +7,7 @@
* replace_empty_jointree
* pull_up_sublinks
* preprocess_function_rtes
+ * expand_virtual_generated_columns
* pull_up_subqueries
* flatten_simple_union_all
* do expression preprocessing (including flattening JOIN alias vars)
@@ -25,6 +26,7 @@
*/
#include "postgres.h"
+#include "access/table.h"
#include "catalog/pg_type.h"
#include "funcapi.h"
#include "miscadmin.h"
@@ -39,7 +41,9 @@
#include "optimizer/tlist.h"
#include "parser/parse_relation.h"
#include "parser/parsetree.h"
+#include "rewrite/rewriteHandler.h"
#include "rewrite/rewriteManip.h"
+#include "utils/rel.h"
typedef struct nullingrel_info
@@ -58,6 +62,8 @@ typedef struct pullup_replace_vars_context
PlannerInfo *root;
List *targetlist; /* tlist of subquery being pulled up */
RangeTblEntry *target_rte; /* RTE of subquery */
+ int result_relation; /* the index of the result relation in the
+ * rewritten query */
Relids relids; /* relids within subquery, as numbered after
* pullup (set only if target_rte->lateral) */
nullingrel_info *nullinfo; /* per-RTE nullingrel info (set only if
@@ -916,6 +922,132 @@ preprocess_function_rtes(PlannerInfo *root)
}
}
+/*
+ * expand_virtual_generated_columns
+ * Expand all virtual generated column references in a query.
+ *
+ * This scans the rangetable for relations with virtual generated columns, and
+ * replaces all Var nodes in the query that reference these columns with the
+ * generation expressions. Note that we do not descend into subqueries; that
+ * is taken care of when the subqueries are planned.
+ *
+ * This has to be done after we have pulled up any SubLinks within the query's
+ * quals; otherwise any virtual generated column references within the SubLinks
+ * that should be transformed into joins wouldn't get expanded.
+ *
+ * Returns a modified copy of the query tree, if any relations with virtual
+ * generated columns are present.
+ */
+Query *
+expand_virtual_generated_columns(PlannerInfo *root)
+{
+ Query *parse = root->parse;
+ int rt_index;
+ ListCell *lc;
+
+ rt_index = 0;
+ foreach(lc, parse->rtable)
+ {
+ RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+ Relation rel;
+ TupleDesc tupdesc;
+
+ ++rt_index;
+
+ /*
+ * Only normal relations can have virtual generated columns.
+ */
+ if (rte->rtekind != RTE_RELATION)
+ continue;
+
+ rel = table_open(rte->relid, NoLock);
+
+ tupdesc = RelationGetDescr(rel);
+ if (tupdesc->constr && tupdesc->constr->has_generated_virtual)
+ {
+ List *tlist = NIL;
+ pullup_replace_vars_context rvcontext;
+
+ for (int i = 0; i < tupdesc->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
+ TargetEntry *tle;
+
+ if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ {
+ Node *defexpr = build_generation_expression(rel, i + 1);
+
+ ChangeVarNodes(defexpr, 1, rt_index, 0);
+
+ tle = makeTargetEntry((Expr *) defexpr, i + 1, 0, false);
+ tlist = lappend(tlist, tle);
+ }
+ else
+ {
+ Var *var;
+
+ var = makeVar(rt_index,
+ i + 1,
+ attr->atttypid,
+ attr->atttypmod,
+ attr->attcollation,
+ 0);
+
+ tle = makeTargetEntry((Expr *) var, i + 1, 0, false);
+ tlist = lappend(tlist, tle);
+ }
+ }
+
+ Assert(list_length(tlist) > 0);
+ Assert(!rte->lateral);
+
+ /*
+ * The relation's targetlist items are now in the appropriate form
+ * to insert into the query, except that we may need to wrap them
+ * in PlaceHolderVars. Set up required context data for
+ * pullup_replace_vars.
+ */
+ rvcontext.root = root;
+ rvcontext.targetlist = tlist;
+ rvcontext.target_rte = rte;
+ rvcontext.result_relation = parse->resultRelation;
+ /* won't need these values */
+ rvcontext.relids = NULL;
+ rvcontext.nullinfo = NULL;
+ /* pass NULL for outer_hasSubLinks */
+ rvcontext.outer_hasSubLinks = NULL;
+ rvcontext.varno = rt_index;
+ /* this flag will be set below, if needed */
+ rvcontext.wrap_non_vars = false;
+ /* initialize cache array with indexes 0 .. length(tlist) */
+ rvcontext.rv_cache = palloc0((list_length(tlist) + 1) *
+ sizeof(Node *));
+
+ /*
+ * If the query uses grouping sets, we need a PlaceHolderVar for
+ * anything that's not a simple Var. Again, this ensures that
+ * expressions retain their separate identity so that they will
+ * match grouping set columns when appropriate. (It'd be
+ * sufficient to wrap values used in grouping set columns, and do
+ * so only in non-aggregated portions of the tlist and havingQual,
+ * but that would require a lot of infrastructure that
+ * pullup_replace_vars hasn't currently got.)
+ */
+ if (parse->groupingSets)
+ rvcontext.wrap_non_vars = true;
+
+ /*
+ * Apply pullup variable replacement throughout the query tree.
+ */
+ parse = (Query *) pullup_replace_vars((Node *) parse, &rvcontext);
+ }
+
+ table_close(rel, NoLock);
+ }
+
+ return parse;
+}
+
/*
* pull_up_subqueries
* Look for subqueries in the rangetable that can be pulled up into
@@ -1197,6 +1329,13 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
*/
preprocess_function_rtes(subroot);
+ /*
+ * Scan the rangetable for relations with virtual generated columns, and
+ * replace all Var nodes in the query that reference these columns with
+ * the generation expressions.
+ */
+ subquery = subroot->parse = expand_virtual_generated_columns(subroot);
+
/*
* Recursively pull up the subquery's subqueries, so that
* pull_up_subqueries' processing is complete for its jointree and
@@ -1274,6 +1413,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
rvcontext.root = root;
rvcontext.targetlist = subquery->targetList;
rvcontext.target_rte = rte;
+ rvcontext.result_relation = 0;
if (rte->lateral)
{
rvcontext.relids = get_relids_in_jointree((Node *) subquery->jointree,
@@ -1834,6 +1974,7 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
rvcontext.root = root;
rvcontext.targetlist = tlist;
rvcontext.target_rte = rte;
+ rvcontext.result_relation = 0;
rvcontext.relids = NULL; /* can't be any lateral references here */
rvcontext.nullinfo = NULL;
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
@@ -1993,6 +2134,7 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
NULL, /* resname */
false)); /* resjunk */
rvcontext.target_rte = rte;
+ rvcontext.result_relation = 0;
/*
* Since this function was reduced to a Const, it doesn't contain any
@@ -2490,6 +2632,10 @@ pullup_replace_vars_callback(Var *var,
bool need_phv;
Node *newnode;
+ /* System columns are not replaced. */
+ if (varattno < InvalidAttrNumber)
+ return (Node *) copyObject(var);
+
/*
* We need a PlaceHolderVar if the Var-to-be-replaced has nonempty
* varnullingrels (unless we find below that the replacement expression is
@@ -2559,6 +2705,18 @@ pullup_replace_vars_callback(Var *var,
rowexpr->location = var->location;
newnode = (Node *) rowexpr;
+ /* Handle any OLD/NEW RETURNING list Vars */
+ if (var->varreturningtype != VAR_RETURNING_DEFAULT)
+ {
+ ReturningExpr *rexpr = makeNode(ReturningExpr);
+
+ rexpr->retlevelsup = 0;
+ rexpr->retold = (var->varreturningtype == VAR_RETURNING_OLD);
+ rexpr->retexpr = (Expr *) newnode;
+
+ newnode = (Node *) rexpr;
+ }
+
/*
* Insert PlaceHolderVar if needed. Notice that we are wrapping one
* PlaceHolderVar around the whole RowExpr, rather than putting one
@@ -2588,6 +2746,39 @@ pullup_replace_vars_callback(Var *var,
/* Make a copy of the tlist item to return */
newnode = (Node *) copyObject(tle->expr);
+ /* Handle any OLD/NEW RETURNING list Vars */
+ if (var->varreturningtype != VAR_RETURNING_DEFAULT)
+ {
+ /*
+ * Copy varreturningtype onto any Vars in the tlist item that
+ * refer to result_relation (which had better be non-zero).
+ */
+ if (rcon->result_relation == 0)
+ elog(ERROR, "variable returning old/new found outside RETURNING list");
+
+ SetVarReturningType((Node *) newnode, rcon->result_relation,
+ 0, var->varreturningtype);
+
+ /*
+ * If the replacement expression in the targetlist is not simply a
+ * Var referencing result_relation, wrap it in a ReturningExpr
+ * node, so that the executor returns NULL if the OLD/NEW row does
+ * not exist.
+ */
+ if (!IsA(newnode, Var) ||
+ ((Var *) newnode)->varno != rcon->result_relation ||
+ ((Var *) newnode)->varlevelsup != var->varlevelsup)
+ {
+ ReturningExpr *rexpr = makeNode(ReturningExpr);
+
+ rexpr->retlevelsup = 0;
+ rexpr->retold = (var->varreturningtype == VAR_RETURNING_OLD);
+ rexpr->retexpr = (Expr *) newnode;
+
+ newnode = (Node *) rexpr;
+ }
+ }
+
/* Insert PlaceHolderVar if needed */
if (need_phv)
{
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index e996bdc0d21..1fa9f8cc55f 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2190,10 +2190,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
* requires special recursion detection if the new quals have sublink
* subqueries, and if we did it in the loop above query_tree_walker would
* then recurse into those quals a second time.
- *
- * Finally, we expand any virtual generated columns. We do this after
- * each table's RLS policies are applied because the RLS policies might
- * also refer to the table's virtual generated columns.
*/
rt_index = 0;
foreach(lc, parsetree->rtable)
@@ -2207,11 +2203,10 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
++rt_index;
- /*
- * Only normal relations can have RLS policies or virtual generated
- * columns.
- */
- if (rte->rtekind != RTE_RELATION)
+ /* Only normal relations can have RLS policies */
+ if (rte->rtekind != RTE_RELATION ||
+ (rte->relkind != RELKIND_RELATION &&
+ rte->relkind != RELKIND_PARTITIONED_TABLE))
continue;
rel = table_open(rte->relid, NoLock);
@@ -2300,16 +2295,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
if (hasSubLinks)
parsetree->hasSubLinks = true;
- /*
- * Expand any references to virtual generated columns of this table.
- * Note that subqueries in virtual generated column expressions are
- * not currently supported, so this cannot add any more sublinks.
- */
- parsetree = (Query *)
- expand_generated_columns_internal((Node *) parsetree,
- rel, rt_index, rte,
- parsetree->resultRelation);
-
table_close(rel, NoLock);
}
@@ -4456,36 +4441,12 @@ expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
{
- Node *defexpr;
- int attnum = i + 1;
- Oid attcollid;
TargetEntry *te;
-
- defexpr = build_column_default(rel, attnum);
- if (defexpr == NULL)
- elog(ERROR, "no generation expression found for column number %d of table \"%s\"",
- attnum, RelationGetRelationName(rel));
-
- /*
- * If the column definition has a collation and it is
- * different from the collation of the generation expression,
- * put a COLLATE clause around the expression.
- */
- attcollid = attr->attcollation;
- if (attcollid && attcollid != exprCollation(defexpr))
- {
- CollateExpr *ce = makeNode(CollateExpr);
-
- ce->arg = (Expr *) defexpr;
- ce->collOid = attcollid;
- ce->location = -1;
-
- defexpr = (Node *) ce;
- }
+ Node *defexpr = build_generation_expression(rel, i + 1);
ChangeVarNodes(defexpr, 1, rt_index, 0);
- te = makeTargetEntry((Expr *) defexpr, attnum, 0, false);
+ te = makeTargetEntry((Expr *) defexpr, i + 1, 0, false);
tlist = lappend(tlist, te);
}
}
@@ -4528,6 +4489,46 @@ expand_generated_columns_in_expr(Node *node, Relation rel, int rt_index)
return node;
}
+/*
+ * Build the generation expression for the virtual generated column.
+ *
+ * Error out if there is no generation expression found for the given column.
+ */
+Node *
+build_generation_expression(Relation rel, int attrno)
+{
+ TupleDesc rd_att = RelationGetDescr(rel);
+ Form_pg_attribute att_tup = TupleDescAttr(rd_att, attrno - 1);
+ Node *defexpr;
+ Oid attcollid;
+
+ Assert(att_tup->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL);
+
+ defexpr = build_column_default(rel, attrno);
+ if (defexpr == NULL)
+ elog(ERROR, "no generation expression found for column number %d of table \"%s\"",
+ attrno, RelationGetRelationName(rel));
+
+ /*
+ * If the column definition has a collation and it is different from the
+ * collation of the generation expression, put a COLLATE clause around the
+ * expression.
+ */
+ attcollid = att_tup->attcollation;
+ if (attcollid && attcollid != exprCollation(defexpr))
+ {
+ CollateExpr *ce = makeNode(CollateExpr);
+
+ ce->arg = (Expr *) defexpr;
+ ce->collOid = attcollid;
+ ce->location = -1;
+
+ defexpr = (Node *) ce;
+ }
+
+ return defexpr;
+}
+
/*
* QueryRewrite -
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 9433548d279..6994b8c5425 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1010,7 +1010,7 @@ SetVarReturningType_walker(Node *node, SetVarReturningType_context *context)
return expression_tree_walker(node, SetVarReturningType_walker, context);
}
-static void
+void
SetVarReturningType(Node *node, int result_relation, int sublevels_up,
VarReturningType returning_type)
{
diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h
index 0ae57ec24a4..df56202777c 100644
--- a/src/include/optimizer/prep.h
+++ b/src/include/optimizer/prep.h
@@ -25,6 +25,7 @@ extern void transform_MERGE_to_join(Query *parse);
extern void replace_empty_jointree(Query *parse);
extern void pull_up_sublinks(PlannerInfo *root);
extern void preprocess_function_rtes(PlannerInfo *root);
+extern Query *expand_virtual_generated_columns(PlannerInfo *root);
extern void pull_up_subqueries(PlannerInfo *root);
extern void flatten_simple_union_all(PlannerInfo *root);
extern void reduce_outer_joins(PlannerInfo *root);
diff --git a/src/include/rewrite/rewriteHandler.h b/src/include/rewrite/rewriteHandler.h
index 88fe13c5f4f..99cab1a3bfa 100644
--- a/src/include/rewrite/rewriteHandler.h
+++ b/src/include/rewrite/rewriteHandler.h
@@ -39,5 +39,6 @@ extern void error_view_not_updatable(Relation view,
const char *detail);
extern Node *expand_generated_columns_in_expr(Node *node, Relation rel, int rt_index);
+extern Node *build_generation_expression(Relation rel, int attrno);
#endif /* REWRITEHANDLER_H */
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
index 5ec475c63e9..466edd7c1c2 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -55,6 +55,9 @@ extern void IncrementVarSublevelsUp(Node *node, int delta_sublevels_up,
extern void IncrementVarSublevelsUp_rtable(List *rtable,
int delta_sublevels_up, int min_sublevels_up);
+extern void SetVarReturningType(Node *node, int result_relation, int sublevels_up,
+ VarReturningType returning_type);
+
extern bool rangeTableEntry_used(Node *node, int rt_index,
int sublevels_up);
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 35638812be9..cb2383469c6 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1398,3 +1398,92 @@ SELECT attrelid, attname, attgenerated FROM pg_attribute WHERE attgenerated NOT
----------+---------+--------------
(0 rows)
+--
+-- test the expansion of virtual generated columns
+--
+create table gtest32 (
+ a int,
+ b int generated always as (a * 2),
+ c int generated always as (10 + 10),
+ d int generated always as (coalesce(a, 100))
+);
+insert into gtest32 values (1), (2);
+-- Ensure that nullingrel bits are propagated into the generation expression
+explain (costs off)
+select sum(t2.b) over (partition by t2.a),
+ sum(t2.c) over (partition by t2.a),
+ sum(t2.d) over (partition by t2.a)
+from gtest32 as t1 left join gtest32 as t2 on (t1.a = t2.a)
+order by t1.a;
+ QUERY PLAN
+------------------------------------------------------
+ Sort
+ Sort Key: t1.a
+ -> WindowAgg
+ -> Sort
+ Sort Key: t2.a
+ -> Merge Left Join
+ Merge Cond: (t1.a = t2.a)
+ -> Sort
+ Sort Key: t1.a
+ -> Seq Scan on gtest32 t1
+ -> Sort
+ Sort Key: t2.a
+ -> Seq Scan on gtest32 t2
+(13 rows)
+
+select sum(t2.b) over (partition by t2.a),
+ sum(t2.c) over (partition by t2.a),
+ sum(t2.d) over (partition by t2.a)
+from gtest32 as t1 left join gtest32 as t2 on (t1.a = t2.a)
+order by t1.a;
+ sum | sum | sum
+-----+-----+-----
+ 2 | 20 | 1
+ 4 | 20 | 2
+(2 rows)
+
+-- Ensure that the generation expressions are wrapped into PHVs if needed
+explain (verbose, costs off)
+select t2.* from gtest32 t1 left join gtest32 t2 on false;
+ QUERY PLAN
+------------------------------------------------------
+ Nested Loop Left Join
+ Output: a, (a * 2), (20), (COALESCE(a, 100))
+ Join Filter: false
+ -> Seq Scan on generated_virtual_tests.gtest32 t1
+ Output: t1.a, t1.b, t1.c, t1.d
+ -> Result
+ Output: a, 20, COALESCE(a, 100)
+ One-Time Filter: false
+(8 rows)
+
+select t2.* from gtest32 t1 left join gtest32 t2 on false;
+ a | b | c | d
+---+---+---+---
+ | | |
+ | | |
+(2 rows)
+
+explain (verbose, costs off)
+select * from gtest32 t group by grouping sets (a, b, c, d) having c = 20;
+ QUERY PLAN
+-----------------------------------------------------
+ HashAggregate
+ Output: a, ((a * 2)), (20), (COALESCE(a, 100))
+ Hash Key: t.a
+ Hash Key: (t.a * 2)
+ Hash Key: 20
+ Hash Key: COALESCE(t.a, 100)
+ Filter: ((20) = 20)
+ -> Seq Scan on generated_virtual_tests.gtest32 t
+ Output: a, (a * 2), 20, COALESCE(a, 100)
+(9 rows)
+
+select * from gtest32 t group by grouping sets (a, b, c, d) having c = 20;
+ a | b | c | d
+---+---+----+---
+ | | 20 |
+(1 row)
+
+drop table gtest32;
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index 34870813910..25b28a83b1b 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -732,3 +732,41 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
-- sanity check of system catalog
SELECT attrelid, attname, attgenerated FROM pg_attribute WHERE attgenerated NOT IN ('', 's', 'v');
+
+--
+-- test the expansion of virtual generated columns
+--
+
+create table gtest32 (
+ a int,
+ b int generated always as (a * 2),
+ c int generated always as (10 + 10),
+ d int generated always as (coalesce(a, 100))
+);
+
+insert into gtest32 values (1), (2);
+
+-- Ensure that nullingrel bits are propagated into the generation expression
+explain (costs off)
+select sum(t2.b) over (partition by t2.a),
+ sum(t2.c) over (partition by t2.a),
+ sum(t2.d) over (partition by t2.a)
+from gtest32 as t1 left join gtest32 as t2 on (t1.a = t2.a)
+order by t1.a;
+
+select sum(t2.b) over (partition by t2.a),
+ sum(t2.c) over (partition by t2.a),
+ sum(t2.d) over (partition by t2.a)
+from gtest32 as t1 left join gtest32 as t2 on (t1.a = t2.a)
+order by t1.a;
+
+-- Ensure that the generation expressions are wrapped into PHVs if needed
+explain (verbose, costs off)
+select t2.* from gtest32 t1 left join gtest32 t2 on false;
+select t2.* from gtest32 t1 left join gtest32 t2 on false;
+
+explain (verbose, costs off)
+select * from gtest32 t group by grouping sets (a, b, c, d) having c = 20;
+select * from gtest32 t group by grouping sets (a, b, c, d) having c = 20;
+
+drop table gtest32;
--
2.43.0
[application/octet-stream] v7-0002-Eliminate-code-duplication-in-replace_rte_variables-callbacks.patch (13.9K, ../CAMbWs4_DVtFfonq7eL6Ocor4apV9dBb6wOhJ5iUZPEddKc-_vg@mail.gmail.com/3-v7-0002-Eliminate-code-duplication-in-replace_rte_variables-callbacks.patch)
download | inline diff:
From 3975038748fa304043c39adb3de92c01c519c82f Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Fri, 21 Feb 2025 14:01:56 +0900
Subject: [PATCH v7 2/2] Eliminate code duplication in replace_rte_variables
callbacks
The callback functions ReplaceVarsFromTargetList_callback and
pullup_replace_vars_callback are both used to replace Vars in an
expression tree that reference a particular RTE with items from a
targetlist, and they both need to expand whole-tuple reference and
deal with OLD/NEW RETURNING list Vars. As a result, currently there
is significant code duplication between these two functions.
This patch introduces a new function, ReplaceVarFromTargetList, to
perform the replacement and calls it from both callback functions,
thereby eliminating code duplication.
---
src/backend/optimizer/prep/prepjointree.c | 137 ++++------------------
src/backend/rewrite/rewriteManip.c | 83 +++++++++----
src/include/rewrite/rewriteManip.h | 9 +-
3 files changed, 91 insertions(+), 138 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 585b4e5f2de..64f4650f0b7 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -2665,127 +2665,38 @@ pullup_replace_vars_callback(Var *var,
/* Just copy the entry and fall through to adjust phlevelsup etc */
newnode = copyObject(rcon->rv_cache[varattno]);
}
- else if (varattno == InvalidAttrNumber)
+ else
{
- /* Must expand whole-tuple reference into RowExpr */
- RowExpr *rowexpr;
- List *colnames;
- List *fields;
- bool save_wrap_non_vars = rcon->wrap_non_vars;
- int save_sublevelsup = context->sublevels_up;
-
- /*
- * If generating an expansion for a var of a named rowtype (ie, this
- * is a plain relation RTE), then we must include dummy items for
- * dropped columns. If the var is RECORD (ie, this is a JOIN), then
- * omit dropped columns. In the latter case, attach column names to
- * the RowExpr for use of the executor and ruleutils.c.
- *
- * In order to be able to cache the results, we always generate the
- * expansion with varlevelsup = 0, and then adjust below if needed.
- */
- expandRTE(rcon->target_rte,
- var->varno, 0 /* not varlevelsup */ ,
- var->varreturningtype, var->location,
- (var->vartype != RECORDOID),
- &colnames, &fields);
- /* Expand the generated per-field Vars, but don't insert PHVs there */
- rcon->wrap_non_vars = false;
- context->sublevels_up = 0; /* to match the expandRTE output */
- fields = (List *) replace_rte_variables_mutator((Node *) fields,
- context);
- rcon->wrap_non_vars = save_wrap_non_vars;
- context->sublevels_up = save_sublevelsup;
-
- rowexpr = makeNode(RowExpr);
- rowexpr->args = fields;
- rowexpr->row_typeid = var->vartype;
- rowexpr->row_format = COERCE_IMPLICIT_CAST;
- rowexpr->colnames = (var->vartype == RECORDOID) ? colnames : NIL;
- rowexpr->location = var->location;
- newnode = (Node *) rowexpr;
-
- /* Handle any OLD/NEW RETURNING list Vars */
- if (var->varreturningtype != VAR_RETURNING_DEFAULT)
- {
- ReturningExpr *rexpr = makeNode(ReturningExpr);
-
- rexpr->retlevelsup = 0;
- rexpr->retold = (var->varreturningtype == VAR_RETURNING_OLD);
- rexpr->retexpr = (Expr *) newnode;
-
- newnode = (Node *) rexpr;
- }
-
/*
- * Insert PlaceHolderVar if needed. Notice that we are wrapping one
- * PlaceHolderVar around the whole RowExpr, rather than putting one
- * around each element of the row. This is because we need the
- * expression to yield NULL, not ROW(NULL,NULL,...) when it is forced
- * to null by an outer join.
+ * Generate the replacement expression. This takes care of expanding
+ * wholerow references and dealing with non-default varreturningtype.
*/
- if (need_phv)
- {
- newnode = (Node *)
- make_placeholder_expr(rcon->root,
- (Expr *) newnode,
- bms_make_singleton(rcon->varno));
- /* cache it with the PHV, and with phlevelsup etc not set yet */
- rcon->rv_cache[InvalidAttrNumber] = copyObject(newnode);
- }
- }
- else
- {
- /* Normal case referencing one targetlist element */
- TargetEntry *tle = get_tle_by_resno(rcon->targetlist, varattno);
-
- if (tle == NULL) /* shouldn't happen */
- elog(ERROR, "could not find attribute %d in subquery targetlist",
- varattno);
-
- /* Make a copy of the tlist item to return */
- newnode = (Node *) copyObject(tle->expr);
-
- /* Handle any OLD/NEW RETURNING list Vars */
- if (var->varreturningtype != VAR_RETURNING_DEFAULT)
- {
- /*
- * Copy varreturningtype onto any Vars in the tlist item that
- * refer to result_relation (which had better be non-zero).
- */
- if (rcon->result_relation == 0)
- elog(ERROR, "variable returning old/new found outside RETURNING list");
-
- SetVarReturningType((Node *) newnode, rcon->result_relation,
- 0, var->varreturningtype);
-
- /*
- * If the replacement expression in the targetlist is not simply a
- * Var referencing result_relation, wrap it in a ReturningExpr
- * node, so that the executor returns NULL if the OLD/NEW row does
- * not exist.
- */
- if (!IsA(newnode, Var) ||
- ((Var *) newnode)->varno != rcon->result_relation ||
- ((Var *) newnode)->varlevelsup != var->varlevelsup)
- {
- ReturningExpr *rexpr = makeNode(ReturningExpr);
-
- rexpr->retlevelsup = 0;
- rexpr->retold = (var->varreturningtype == VAR_RETURNING_OLD);
- rexpr->retexpr = (Expr *) newnode;
-
- newnode = (Node *) rexpr;
- }
- }
+ newnode = ReplaceVarFromTargetList(var,
+ rcon->target_rte,
+ rcon->targetlist,
+ rcon->result_relation,
+ REPLACEVARS_REPORT_ERROR,
+ 0);
/* Insert PlaceHolderVar if needed */
if (need_phv)
{
bool wrap;
- if (newnode && IsA(newnode, Var) &&
- ((Var *) newnode)->varlevelsup == 0)
+ if (varattno == InvalidAttrNumber)
+ {
+ /*
+ * Insert PlaceHolderVar for whole-tuple reference. Notice
+ * that we are wrapping one PlaceHolderVar around the whole
+ * RowExpr, rather than putting one around each element of the
+ * row. This is because we need the expression to yield NULL,
+ * not ROW(NULL,NULL,...) when it is forced to null by an
+ * outer join.
+ */
+ wrap = true;
+ }
+ else if (newnode && IsA(newnode, Var) &&
+ ((Var *) newnode)->varlevelsup == 0)
{
/*
* Simple Vars always escape being wrapped, unless they are
@@ -2931,7 +2842,7 @@ pullup_replace_vars_callback(Var *var,
* Cache it if possible (ie, if the attno is in range, which
* it probably always should be).
*/
- if (varattno > InvalidAttrNumber &&
+ if (varattno >= InvalidAttrNumber &&
varattno <= list_length(rcon->targetlist))
rcon->rv_cache[varattno] = copyObject(newnode);
}
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 6994b8c5425..6e6ea76a664 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1010,7 +1010,7 @@ SetVarReturningType_walker(Node *node, SetVarReturningType_context *context)
return expression_tree_walker(node, SetVarReturningType_walker, context);
}
-void
+static void
SetVarReturningType(Node *node, int result_relation, int sublevels_up,
VarReturningType returning_type)
{
@@ -1814,6 +1814,30 @@ ReplaceVarsFromTargetList_callback(Var *var,
replace_rte_variables_context *context)
{
ReplaceVarsFromTargetList_context *rcon = (ReplaceVarsFromTargetList_context *) context->callback_arg;
+ Node *newnode;
+
+ newnode = ReplaceVarFromTargetList(var,
+ rcon->target_rte,
+ rcon->targetlist,
+ rcon->result_relation,
+ rcon->nomatch_option,
+ rcon->nomatch_varno);
+
+ /* Must adjust varlevelsup if replaced Var is within a subquery */
+ if (var->varlevelsup > 0)
+ IncrementVarSublevelsUp(newnode, var->varlevelsup, 0);
+
+ return newnode;
+}
+
+Node *
+ReplaceVarFromTargetList(Var *var,
+ RangeTblEntry *target_rte,
+ List *targetlist,
+ int result_relation,
+ ReplaceVarsNoMatchOption nomatch_option,
+ int nomatch_varno)
+{
TargetEntry *tle;
if (var->varattno == InvalidAttrNumber)
@@ -1822,6 +1846,7 @@ ReplaceVarsFromTargetList_callback(Var *var,
RowExpr *rowexpr;
List *colnames;
List *fields;
+ ListCell *lc;
/*
* If generating an expansion for a var of a named rowtype (ie, this
@@ -1830,29 +1855,46 @@ ReplaceVarsFromTargetList_callback(Var *var,
* omit dropped columns. In the latter case, attach column names to
* the RowExpr for use of the executor and ruleutils.c.
*
+ * In order to be able to cache the results, we always generate the
+ * expansion with varlevelsup = 0. The caller is responsible for
+ * adjusting it if needed.
+ *
* The varreturningtype is copied onto each individual field Var, so
* that it is handled correctly when we recurse.
*/
- expandRTE(rcon->target_rte,
- var->varno, var->varlevelsup, var->varreturningtype,
- var->location, (var->vartype != RECORDOID),
+ expandRTE(target_rte,
+ var->varno, 0 /* not varlevelsup */ ,
+ var->varreturningtype, var->location,
+ (var->vartype != RECORDOID),
&colnames, &fields);
- /* Adjust the generated per-field Vars... */
- fields = (List *) replace_rte_variables_mutator((Node *) fields,
- context);
rowexpr = makeNode(RowExpr);
- rowexpr->args = fields;
+ /* the fields will be set below */
+ rowexpr->args = NIL;
rowexpr->row_typeid = var->vartype;
rowexpr->row_format = COERCE_IMPLICIT_CAST;
rowexpr->colnames = (var->vartype == RECORDOID) ? colnames : NIL;
rowexpr->location = var->location;
+ /* Adjust the generated per-field Vars... */
+ foreach(lc, fields)
+ {
+ Node *field = lfirst(lc);
+
+ if (field && IsA(field, Var))
+ field = ReplaceVarFromTargetList((Var *) field,
+ target_rte,
+ targetlist,
+ result_relation,
+ nomatch_option,
+ nomatch_varno);
+ rowexpr->args = lappend(rowexpr->args, field);
+ }
/* Wrap it in a ReturningExpr, if needed, per comments above */
if (var->varreturningtype != VAR_RETURNING_DEFAULT)
{
ReturningExpr *rexpr = makeNode(ReturningExpr);
- rexpr->retlevelsup = var->varlevelsup;
+ rexpr->retlevelsup = 0;
rexpr->retold = (var->varreturningtype == VAR_RETURNING_OLD);
rexpr->retexpr = (Expr *) rowexpr;
@@ -1863,12 +1905,12 @@ ReplaceVarsFromTargetList_callback(Var *var,
}
/* Normal case referencing one targetlist element */
- tle = get_tle_by_resno(rcon->targetlist, var->varattno);
+ tle = get_tle_by_resno(targetlist, var->varattno);
if (tle == NULL || tle->resjunk)
{
/* Failed to find column in targetlist */
- switch (rcon->nomatch_option)
+ switch (nomatch_option)
{
case REPLACEVARS_REPORT_ERROR:
/* fall through, throw error below */
@@ -1876,7 +1918,8 @@ ReplaceVarsFromTargetList_callback(Var *var,
case REPLACEVARS_CHANGE_VARNO:
var = copyObject(var);
- var->varno = rcon->nomatch_varno;
+ var->varno = nomatch_varno;
+ var->varlevelsup = 0;
/* we leave the syntactic referent alone */
return (Node *) var;
@@ -1906,10 +1949,6 @@ ReplaceVarsFromTargetList_callback(Var *var,
/* Make a copy of the tlist item to return */
Expr *newnode = copyObject(tle->expr);
- /* Must adjust varlevelsup if tlist item is from higher query */
- if (var->varlevelsup > 0)
- IncrementVarSublevelsUp((Node *) newnode, var->varlevelsup, 0);
-
/*
* Check to see if the tlist item contains a PARAM_MULTIEXPR Param,
* and throw error if so. This case could only happen when expanding
@@ -1932,20 +1971,20 @@ ReplaceVarsFromTargetList_callback(Var *var,
* Copy varreturningtype onto any Vars in the tlist item that
* refer to result_relation (which had better be non-zero).
*/
- if (rcon->result_relation == 0)
+ if (result_relation == 0)
elog(ERROR, "variable returning old/new found outside RETURNING list");
- SetVarReturningType((Node *) newnode, rcon->result_relation,
- var->varlevelsup, var->varreturningtype);
+ SetVarReturningType((Node *) newnode, result_relation,
+ 0, var->varreturningtype);
/* Wrap it in a ReturningExpr, if needed, per comments above */
if (!IsA(newnode, Var) ||
- ((Var *) newnode)->varno != rcon->result_relation ||
- ((Var *) newnode)->varlevelsup != var->varlevelsup)
+ ((Var *) newnode)->varno != result_relation ||
+ ((Var *) newnode)->varlevelsup != 0)
{
ReturningExpr *rexpr = makeNode(ReturningExpr);
- rexpr->retlevelsup = var->varlevelsup;
+ rexpr->retlevelsup = 0;
rexpr->retold = (var->varreturningtype == VAR_RETURNING_OLD);
rexpr->retexpr = newnode;
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
index 466edd7c1c2..ea3908739c6 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -55,9 +55,6 @@ extern void IncrementVarSublevelsUp(Node *node, int delta_sublevels_up,
extern void IncrementVarSublevelsUp_rtable(List *rtable,
int delta_sublevels_up, int min_sublevels_up);
-extern void SetVarReturningType(Node *node, int result_relation, int sublevels_up,
- VarReturningType returning_type);
-
extern bool rangeTableEntry_used(Node *node, int rt_index,
int sublevels_up);
@@ -92,6 +89,12 @@ extern Node *map_variable_attnos(Node *node,
const struct AttrMap *attno_map,
Oid to_rowtype, bool *found_whole_row);
+extern Node *ReplaceVarFromTargetList(Var *var,
+ RangeTblEntry *target_rte,
+ List *targetlist,
+ int result_relation,
+ ReplaceVarsNoMatchOption nomatch_option,
+ int nomatch_varno);
extern Node *ReplaceVarsFromTargetList(Node *node,
int target_varno, int sublevels_up,
RangeTblEntry *target_rte,
--
2.43.0
view thread (29+ 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], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Virtual generated columns
In-Reply-To: <CAMbWs4_DVtFfonq7eL6Ocor4apV9dBb6wOhJ5iUZPEddKc-_vg@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