On Tue, 18 Feb 2025 at 13:12, Richard Guo <guofengli...@gmail.com> wrote:
>
> > It seems to me that, for a relation in the rangetable that has virtual
> > generated columns, we can consider it a subquery to some extent. For
> > instance, suppose we have a query:
> >
> > select ... from ... join t on ...;
> >
> > and suppose t.b is a virtual generated column. We can consider this
> > query as:
> >
> > select ... from ... join (select a, expr() as b from t) as t on ...;
> >
> > In this sense, I'm wondering if we can leverage the
> > pullup_replace_vars architecture to expand the virtual generated
> > columns. I believe this would help avoid a lot of duplicate code with
> > pullup_replace_vars_callback.
Yes, I think this is much better. Having just one place that does that
complex logic is a definite win.
At one point I had the idea of making the rewriter turn RTEs with
virtual generated columns into subquery RTEs, so then the planner
would treat them just like views, but that would have been less
efficient. Also, I think there would have been a problem with RLS
quals, which would have been added to the subquery RTEs. Perhaps that
could have been fixed up during subquery pullup, but it felt ugly and
I didn't actually try it.
> I had a try with this idea, and attached is what I came up with. It
> fixes all the mentioned issues but still requires significant
> refinement, particularly due to the lack of comments. By leveraging
> the pullup_replace_vars architecture to expand the virtual generated
> columns, it saves a lot of duplicate code.
One thing I don't like about this is that it's introducing more code
duplication between pullup_replace_vars() and
ReplaceVarsFromTargetList(). Those already had a lot of code in common
before RETURNING OLD/NEW was added, and this is duplicating even more
code. I think it'd be better to refactor so that they share common
code, since it has become quite complex, and it would be better to
have just one place to maintain. Attached is an updated patch doing
that.
Regards,
Dean
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 7b1a8a0..52c6d11
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -750,6 +750,11 @@ subquery_planner(PlannerGlobal *glob, Qu
flatten_simple_union_all(root);
/*
+ * Expand virtual generated columns. XXX more comments
+ */
+ parse = root->parse = expand_virtual_generated_columns(root);
+
+ /*
* Survey the rangetable to see what kinds of entries are present. We can
* skip some later processing if relevant SQL features are not used; for
* example if there are no JOIN RTEs we can avoid the expense of doing
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
new file mode 100644
index 5d9225e..0eb7715
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -9,6 +9,7 @@
* preprocess_function_rtes
* pull_up_subqueries
* flatten_simple_union_all
+ * expand_virtual_generated_columns
* do expression preprocessing (including flattening JOIN alias vars)
* reduce_outer_joins
* remove_useless_result_rtes
@@ -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
@@ -57,6 +61,7 @@ typedef struct pullup_replace_vars_conte
{
PlannerInfo *root;
List *targetlist; /* tlist of subquery being pulled up */
+ int result_relation; /* the index of the result relation */
RangeTblEntry *target_rte; /* RTE of subquery */
Relids relids; /* relids within subquery, as numbered after
* pullup (set only if target_rte->lateral) */
@@ -1273,6 +1278,7 @@ pull_up_simple_subquery(PlannerInfo *roo
*/
rvcontext.root = root;
rvcontext.targetlist = subquery->targetList;
+ rvcontext.result_relation = 0;
rvcontext.target_rte = rte;
if (rte->lateral)
{
@@ -1833,6 +1839,7 @@ pull_up_simple_values(PlannerInfo *root,
}
rvcontext.root = root;
rvcontext.targetlist = tlist;
+ rvcontext.result_relation = 0;
rvcontext.target_rte = rte;
rvcontext.relids = NULL; /* can't be any lateral references here */
rvcontext.nullinfo = NULL;
@@ -1992,6 +1999,7 @@ pull_up_constant_function(PlannerInfo *r
1, /* resno */
NULL, /* resname */
false)); /* resjunk */
+ rvcontext.result_relation = 0;
rvcontext.target_rte = rte;
/*
@@ -2499,6 +2507,10 @@ pullup_replace_vars_callback(Var *var,
*/
need_phv = (var->varnullingrels != NULL) || rcon->wrap_non_vars;
+ /* System columns are not replaced. */
+ if (varattno < InvalidAttrNumber)
+ return (Node *) copyObject(var);
+
/*
* If PlaceHolderVars are needed, we cache the modified expressions in
* rcon->rv_cache[]. This is not in hopes of any material speed gain
@@ -2516,85 +2528,43 @@ pullup_replace_vars_callback(Var *var,
varattno <= list_length(rcon->targetlist) &&
rcon->rv_cache[varattno] != NULL)
{
- /* Just copy the entry and fall through to adjust phlevelsup etc */
+ /* Copy the cached item and adjust its varlevelsup */
newnode = copyObject(rcon->rv_cache[varattno]);
+ if (var->varlevelsup > 0)
+ IncrementVarSublevelsUp(newnode, var->varlevelsup, 0);
}
- 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;
-
/*
- * 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, as well as adjusting varlevelsup and
+ * 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);
+ newnode = ReplaceVarFromTargetList(var,
+ rcon->target_rte,
+ rcon->targetlist,
+ rcon->result_relation,
+ REPLACEVARS_REPORT_ERROR,
+ var->varno);
/* Insert PlaceHolderVar if needed */
if (need_phv)
{
bool wrap;
- if (newnode && IsA(newnode, Var) &&
- ((Var *) newnode)->varlevelsup == 0)
+ if (varattno == InvalidAttrNumber)
+ {
+ /*
+ * A wholerow Var is expanded to a RowExpr, which we wrap with
+ * a single PlaceHolderVar, 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 == var->varlevelsup)
{
/*
* Simple Vars always escape being wrapped, unless they are
@@ -2616,7 +2586,7 @@ pullup_replace_vars_callback(Var *var,
}
}
else if (newnode && IsA(newnode, PlaceHolderVar) &&
- ((PlaceHolderVar *) newnode)->phlevelsup == 0)
+ ((PlaceHolderVar *) newnode)->phlevelsup == var->varlevelsup)
{
/* The same rules apply for a PlaceHolderVar */
wrap = false;
@@ -2735,14 +2705,25 @@ pullup_replace_vars_callback(Var *var,
make_placeholder_expr(rcon->root,
(Expr *) newnode,
bms_make_singleton(rcon->varno));
+ ((PlaceHolderVar *) newnode)->phlevelsup = var->varlevelsup;
/*
* Cache it if possible (ie, if the attno is in range, which
- * it probably always should be).
+ * it probably always should be), ensuring that the cached
+ * item has phlevelsup = 0.
*/
- if (varattno > InvalidAttrNumber &&
+ if (varattno >= InvalidAttrNumber &&
varattno <= list_length(rcon->targetlist))
- rcon->rv_cache[varattno] = copyObject(newnode);
+ {
+ Node *cachenode = copyObject(newnode);
+
+ if (var->varlevelsup > 0)
+ IncrementVarSublevelsUp(cachenode,
+ -((int) var->varlevelsup),
+ 0);
+
+ rcon->rv_cache[varattno] = cachenode;
+ }
}
}
}
@@ -2754,7 +2735,7 @@ pullup_replace_vars_callback(Var *var,
{
Var *newvar = (Var *) newnode;
- Assert(newvar->varlevelsup == 0);
+ Assert(newvar->varlevelsup == var->varlevelsup);
newvar->varnullingrels = bms_add_members(newvar->varnullingrels,
var->varnullingrels);
}
@@ -2762,7 +2743,7 @@ pullup_replace_vars_callback(Var *var,
{
PlaceHolderVar *newphv = (PlaceHolderVar *) newnode;
- Assert(newphv->phlevelsup == 0);
+ Assert(newphv->phlevelsup == var->varlevelsup);
newphv->phnullingrels = bms_add_members(newphv->phnullingrels,
var->varnullingrels);
}
@@ -2825,10 +2806,6 @@ pullup_replace_vars_callback(Var *var,
}
}
- /* Must adjust varlevelsup if replaced Var is within a subquery */
- if (var->varlevelsup > 0)
- IncrementVarSublevelsUp(newnode, var->varlevelsup, 0);
-
return newnode;
}
@@ -2947,6 +2924,128 @@ flatten_simple_union_all(PlannerInfo *ro
}
+/*
+ * expand_virtual_generated_columns
+ * Expand all virtual generated column references in a query.
+ *
+ * XXX more comments
+ */
+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);
+ int attnum = i + 1;
+ TargetEntry *te;
+
+ if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ {
+ Node *defexpr;
+ Oid attcollid;
+
+ 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;
+ }
+
+ ChangeVarNodes(defexpr, 1, rt_index, 0);
+
+ te = makeTargetEntry((Expr *) defexpr, attnum, 0, false);
+ tlist = lappend(tlist, te);
+ }
+ else
+ {
+ Var *var;
+
+ var = makeVar(rt_index,
+ attnum,
+ attr->atttypid,
+ attr->atttypmod,
+ attr->attcollation,
+ 0);
+ te = makeTargetEntry((Expr *) var, attnum, 0, false);
+ tlist = lappend(tlist, te);
+ }
+ }
+
+ Assert(list_length(tlist) > 0);
+
+ rvcontext.root = root;
+ rvcontext.targetlist = tlist;
+ rvcontext.result_relation = parse->resultRelation;
+ rvcontext.target_rte = rte;
+ rvcontext.relids = NULL;
+ rvcontext.nullinfo = NULL;
+ rvcontext.outer_hasSubLinks = NULL;
+ rvcontext.varno = rt_index;
+ rvcontext.wrap_non_vars = false;
+ 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. This ensures that
+ * expressions retain their separate identity so that they will
+ * match grouping set columns when appropriate.
+ */
+ if (parse->groupingSets)
+ rvcontext.wrap_non_vars = true;
+
+ parse = (Query *) pullup_replace_vars((Node *) parse, &rvcontext);
+ }
+
+ table_close(rel, NoLock);
+ }
+
+ return parse;
+}
+
+
/*
* reduce_outer_joins
* Attempt to reduce outer joins to plain inner joins.
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index e996bdc..7a39abd
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2190,10 +2190,6 @@ fireRIRrules(Query *parsetree, List *act
* 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 *act
++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 *act
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);
}
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
new file mode 100644
index a115b21..cc268a5
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1736,6 +1736,23 @@ ReplaceVarsFromTargetList_callback(Var *
replace_rte_variables_context *context)
{
ReplaceVarsFromTargetList_context *rcon = (ReplaceVarsFromTargetList_context *) context->callback_arg;
+
+ return ReplaceVarFromTargetList(var,
+ rcon->target_rte,
+ rcon->targetlist,
+ rcon->result_relation,
+ rcon->nomatch_option,
+ rcon->nomatch_varno);
+}
+
+Node *
+ReplaceVarFromTargetList(Var *var,
+ RangeTblEntry *target_rte,
+ List *targetlist,
+ int result_relation,
+ ReplaceVarsNoMatchOption nomatch_option,
+ int nomatch_varno)
+{
TargetEntry *tle;
if (var->varattno == InvalidAttrNumber)
@@ -1744,6 +1761,7 @@ ReplaceVarsFromTargetList_callback(Var *
RowExpr *rowexpr;
List *colnames;
List *fields;
+ ListCell *lc;
/*
* If generating an expansion for a var of a named rowtype (ie, this
@@ -1755,15 +1773,27 @@ ReplaceVarsFromTargetList_callback(Var *
* The varreturningtype is copied onto each individual field Var, so
* that it is handled correctly when we recurse.
*/
- expandRTE(rcon->target_rte,
+ expandRTE(target_rte,
var->varno, var->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;
+ rowexpr->args = NIL;
+ foreach(lc, fields)
+ {
+ Node *field = lfirst(lc);
+
+ if (field != NULL)
+ field = ReplaceVarFromTargetList((Var *) field,
+ target_rte,
+ targetlist,
+ result_relation,
+ nomatch_option,
+ nomatch_varno);
+
+ rowexpr->args = lappend(rowexpr->args, field);
+ }
rowexpr->row_typeid = var->vartype;
rowexpr->row_format = COERCE_IMPLICIT_CAST;
rowexpr->colnames = (var->vartype == RECORDOID) ? colnames : NIL;
@@ -1785,12 +1815,12 @@ ReplaceVarsFromTargetList_callback(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 */
@@ -1798,7 +1828,7 @@ ReplaceVarsFromTargetList_callback(Var *
case REPLACEVARS_CHANGE_VARNO:
var = copyObject(var);
- var->varno = rcon->nomatch_varno;
+ var->varno = nomatch_varno;
/* we leave the syntactic referent alone */
return (Node *) var;
@@ -1854,15 +1884,15 @@ ReplaceVarsFromTargetList_callback(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,
+ SetVarReturningType((Node *) newnode, result_relation,
var->varlevelsup, var->varreturningtype);
/* Wrap it in a ReturningExpr, if needed, per comments above */
if (!IsA(newnode, Var) ||
- ((Var *) newnode)->varno != rcon->result_relation ||
+ ((Var *) newnode)->varno != result_relation ||
((Var *) newnode)->varlevelsup != var->varlevelsup)
{
ReturningExpr *rexpr = makeNode(ReturningExpr);
diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h
new file mode 100644
index 0ae57ec..6bc75ac
--- a/src/include/optimizer/prep.h
+++ b/src/include/optimizer/prep.h
@@ -27,6 +27,7 @@ extern void pull_up_sublinks(PlannerInfo
extern void preprocess_function_rtes(PlannerInfo *root);
extern void pull_up_subqueries(PlannerInfo *root);
extern void flatten_simple_union_all(PlannerInfo *root);
+extern Query *expand_virtual_generated_columns(PlannerInfo *root);
extern void reduce_outer_joins(PlannerInfo *root);
extern void remove_useless_result_rtes(PlannerInfo *root);
extern Relids get_relids_in_jointree(Node *jtnode, bool include_outer_joins,
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
new file mode 100644
index 5128230..afce743
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -85,6 +85,13 @@ extern Node *map_variable_attnos(Node *n
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,
