Subject: Re: Virtual generated columns

From: jian he <[email-masked]>
Date: Mon, 10 Feb 2025 12:52:45 +0800

On Mon, Feb 10, 2025 at 11:54 AM Richard Guo <[email-masked]> wrote:
>
> On Sun, Feb 9, 2025 at 7:02 PM Zhang Mingli <[email-masked]> wrote:
> > On Feb 9, 2025 at 16:00 +0800, Alexander Lakhin <[email-masked]>, wrote:
> > Please look at a planner error with a virtual generated column triggered
> > by the following script:
> > CREATE TABLE t(a int, b int GENERATED ALWAYS AS (a * 1));
> >
> > SELECT SUM(CASE WHEN t.b = 1 THEN 1 ELSE 1 END) OVER (PARTITION BY t.a)
> > FROM t AS t1 LEFT JOIN T ON true;
> >
> > ERROR:  XX000: wrong varnullingrels (b) (expected (b 3)) for Var 2/1
> > LOCATION:  search_indexed_tlist_for_var, setrefs.c:2901
>
> > During the parse stage, we set the Var->varnullingrels in the parse_analyze_fixedparams function.
> > Later, when rewriting the parse tree in pg_rewrite_query() to expand virtual columns, we replace the expression column b with a new Var that includes a, since b is defined as a * 1.
> > Unfortunately, we overlooked updating the Var->varnullingrels at this point.
> > As a result, when we enter search_indexed_tlist_for_var, it leads to a failure.
> > While we do have another target entry with the correct varnullingrels, the expression involving the virtual column generates another column reference, which causes the error.
> > Currently, I don't have a solid fix.
> > One potential solution is to correct the Vars at or after the rewrite stage by traversing the parse tree again using markNullableIfNeeded.
> > However, this approach may require exposing the ParseState, which doesn't seem ideal.
> > It appears that the virtual column generation function during the rewrite stage does not account for the Var field settings, leading to the errors we are encountering.
>
> Hmm, would it be possible to propagate any varnullingrels into the
> replacement expression in ReplaceVarsFromTargetList_callback()?
>
in ReplaceVarsFromTargetList_callback,
we have
``if (var->varlevelsup > 0)``
``if (var->varreturningtype != VAR_RETURNING_DEFAULT)``

we can also have.
 ``if (var->varnullingrels != NULL)``

please check attached.

> BTW, I was curious about what happens if the replacement expression is
> constant, so I tried running the query below.
>
> CREATE TABLE t (a int, b int GENERATED ALWAYS AS (1 + 1));
> INSERT INTO t VALUES (1);
> INSERT INTO t VALUES (2);
>
> # SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE;
>  a | b
> ---+---
>    | 2
>    | 2
> (2 rows)
>
> Is this the expected behavior?  I was expecting that t2.b should be
> all NULLs.
>
SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE;
should be same as
SELECT t2.a,  2 as b FROM t t1 LEFT JOIN t t2 ON FALSE;
so i think this is expected.
From 78f272701afb23994021a0b98fa6deaf9d37cc04 Mon Sep 17 00:00:00 2001
From: jian he <[email-masked]>
Date: Mon, 10 Feb 2025 12:51:17 +0800
Subject: [PATCH v1 1/1] fix expand virtual generated column Var node
 varnullingrels field

discussion: https://postgr.es/m/[email-masked]
---
 src/backend/rewrite/rewriteManip.c            | 54 +++++++++++++++++++
 src/include/rewrite/rewriteManip.h            |  2 +
 .../regress/expected/generated_virtual.out    | 37 +++++++++++++
 src/test/regress/sql/generated_virtual.sql    |  9 ++++
 src/tools/pgindent/typedefs.list              |  1 +
 5 files changed, 103 insertions(+)

diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index a115b217c9..9d81d5441a 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -766,6 +766,41 @@ typedef struct
 	int			min_sublevels_up;
 } IncrementVarSublevelsUp_context;
 
+typedef struct
+{
+	int			sublevels_up;
+	Bitmapset *varnullingrels;
+} SetVarNullingrels_context;
+
+static bool
+SetVarNullingrels_walker(Node *node,
+						 SetVarNullingrels_context *context)
+{
+	if (node == NULL)
+		return false;
+	if (IsA(node, Var))
+	{
+		Var		   *var = (Var *) node;
+
+		if (var->varlevelsup == context->sublevels_up)
+			var->varnullingrels = bms_union(context->varnullingrels, var->varnullingrels);
+
+		return false;
+	}
+	if (IsA(node, Query))
+	{
+		/* Recurse into subselects */
+		bool		result;
+
+		context->sublevels_up++;
+		result = query_tree_walker((Query *) node, SetVarNullingrels_walker,
+								   context, 0);
+		context->sublevels_up--;
+		return result;
+	}
+	return expression_tree_walker(node, SetVarNullingrels_walker, context);
+}
+
 static bool
 IncrementVarSublevelsUp_walker(Node *node,
 							   IncrementVarSublevelsUp_context *context)
@@ -846,6 +881,21 @@ IncrementVarSublevelsUp_walker(Node *node,
 	return expression_tree_walker(node, IncrementVarSublevelsUp_walker, context);
 }
 
+void
+SetVarNullingrels(Node *node, int sublevels_up, Bitmapset *varnullingrels)
+{
+	SetVarNullingrels_context context;
+
+	context.sublevels_up = sublevels_up;
+	context.varnullingrels = varnullingrels;
+
+	/* Expect to start with an expression */
+	query_or_expression_tree_walker(node,
+									SetVarNullingrels_walker,
+									&context,
+									QTW_EXAMINE_RTES_BEFORE);
+}
+
 void
 IncrementVarSublevelsUp(Node *node, int delta_sublevels_up,
 						int min_sublevels_up)
@@ -1832,6 +1882,10 @@ ReplaceVarsFromTargetList_callback(Var *var,
 		if (var->varlevelsup > 0)
 			IncrementVarSublevelsUp((Node *) newnode, var->varlevelsup, 0);
 
+		if (var->varnullingrels != NULL)
+			SetVarNullingrels((Node *) newnode, var->varlevelsup,
+							  var->varnullingrels);
+
 		/*
 		 * Check to see if the tlist item contains a PARAM_MULTIEXPR Param,
 		 * and throw error if so.  This case could only happen when expanding
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
index 512823033b..cca38a6fd7 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -48,6 +48,8 @@ extern void ChangeVarNodes(Node *node, int rt_index, int new_index,
 						   int sublevels_up);
 extern void IncrementVarSublevelsUp(Node *node, int delta_sublevels_up,
 									int min_sublevels_up);
+extern void SetVarNullingrels(Node *node, int sublevels_up,
+							  Bitmapset *varnullingrels);
 extern void IncrementVarSublevelsUp_rtable(List *rtable,
 										   int delta_sublevels_up, int min_sublevels_up);
 
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 8660904a34..d6fe82fb13 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -181,6 +181,43 @@ SELECT * FROM gtestx, gtest1 WHERE gtestx.y = gtest1.a;
  22 | 2 | 2 | 4
 (2 rows)
 
+--bug: https://postgr.es/m/[email-masked]
+SELECT SUM(t.b) OVER (PARTITION BY t.a) FROM gtestx AS t1 left JOIN gtest1 T ON true;
+ sum 
+-----
+   6
+   6
+   6
+  12
+  12
+  12
+(6 rows)
+
+--this query result should be same as as the above
+SELECT SUM((select t.b from ((select t.b from gtest1 as t order by t.b limit 1)))) OVER (PARTITION BY t.a)
+FROM gtestx AS t1 left JOIN gtest1 T ON true;
+ sum 
+-----
+   6
+   6
+   6
+  12
+  12
+  12
+(6 rows)
+
+SELECT SUM((select t.b from gtest1 as t order by t.b limit 1)) OVER (PARTITION BY t.a)
+FROM gtestx AS t1 left JOIN gtest1 T ON true;
+ sum 
+-----
+   6
+   6
+   6
+   6
+   6
+   6
+(6 rows)
+
 DROP TABLE gtestx;
 -- test UPDATE/DELETE quals
 SELECT * FROM gtest1 ORDER BY a;
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index 3207e5cb1f..10638b29f8 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -80,6 +80,15 @@ DELETE FROM gtest1 WHERE a = 2000000000;
 CREATE TABLE gtestx (x int, y int);
 INSERT INTO gtestx VALUES (11, 1), (22, 2), (33, 3);
 SELECT * FROM gtestx, gtest1 WHERE gtestx.y = gtest1.a;
+--bug: https://postgr.es/m/[email-masked]
+SELECT SUM(t.b) OVER (PARTITION BY t.a) FROM gtestx AS t1 left JOIN gtest1 T ON true;
+--this query result should be same as as the above
+SELECT SUM((select t.b from ((select t.b from gtest1 as t order by t.b limit 1)))) OVER (PARTITION BY t.a)
+FROM gtestx AS t1 left JOIN gtest1 T ON true;
+
+SELECT SUM((select t.b from gtest1 as t order by t.b limit 1)) OVER (PARTITION BY t.a)
+FROM gtestx AS t1 left JOIN gtest1 T ON true;
+
 DROP TABLE gtestx;
 
 -- test UPDATE/DELETE quals
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9a3bee93de..a9e15be55f 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2632,6 +2632,7 @@ SetOperation
 SetOperationStmt
 SetQuantifier
 SetToDefault
+SetVarNullingrels_context
 SetVarReturningType_context
 SetupWorkerPtrType
 ShDependObjectInfo
-- 
2.34.1

---

From: Zhang Mingli <[email-masked]>
Date: Mon, 10 Feb 2025 13:15:58 +0800

> so i think this is expected.
Hi,

I believe virtual columns should behave like stored columns, except they don't actually use storage.
Virtual columns are computed when the table is read, and they should adhere to the same rules of join semantics.
I agree with Richard, the result seems incorrect. The right outcome should be:
gpadmin=# SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE;
 a | b
------+------
 NULL | NULL
 NULL | NULL
(2 rows)

--
Zhang Mingli
HashData

---

From: Richard Guo <[email-masked]>
Date: Tue, 11 Feb 2025 10:34:10 +0800

> gpadmin=# SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE;

Yeah, I also feel that the virtual generated columns should adhere to
outer join semantics, rather than being unconditionally replaced by
the generation expressions.  But maybe I'm wrong.

If that's the case, this incorrect-result issue isn't limited to
constant expressions; it could also occur with non-strict ones.

CREATE TABLE t (a int, b int GENERATED ALWAYS AS (COALESCE(a, 100)));
INSERT INTO t VALUES (1);
INSERT INTO t VALUES (2);

---+-----
   | 100
   | 100
(2 rows)

It seems to me that virtual generated columns should be expanded in
the planner rather than in the rewriter.  Additionally, we may need to
wrap the replacement expressions in PHVs if the virtual generated
columns come from the nullable side of an outer join, similar to what
we do when pulling up subqueries.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Tue, 11 Feb 2025 17:15:24 +0800

> constant expressions; it could also occur with non-strict ones.

It seems that outer-join removal does not work well with virtual
generated columns.

create table t (a int, b int);
create table vt (a int primary key, b int generated always as (a * 2));

explain (costs off)
select t.a from t left join vt on t.a = vt.a where coalesce(vt.b, 1) = 1;
  QUERY PLAN
---------------
 Seq Scan on t
(1 row)

This plan does not seem correct to me.  The inner-rel attribute 'vt.b'
is used above the join, which means the join should not be removed.

explain (costs off)
select t.a from t left join vt on t.a = vt.a where coalesce(vt.b, 1) =
1 or t.a is null;
server closed the connection unexpectedly

For this query, an Assert in remove_rel_from_query() is hit.

I haven't looked into the details yet, but I suspect that both of
these issues are caused by our failure to mark the correct nullingrel
bits for the virtual generated columns.

Thanks
Richard

---

From: jian he <[email-masked]>
Date: Thu, 13 Feb 2025 21:06:59 +0800

> # SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE;

Now I agree with you.
I think the following two should return the same result.

SELECT t2.a, t2.b FROM t t1 LEFT JOIN t t2 ON FALSE;
SELECT t2.a, t2.b FROM t t1 LEFT JOIN (select * from t) t2 ON FALSE;

------------------------
atatch refined patch solves the failure to copy the nullingrel bits
for the virtual generated columns.
in ReplaceVarsFromTargetList_callback.
I tried to just use add_nulling_relids, but failed, so I did the
similar thing as SetVarReturningType.

I didn't solve the out join semantic issue.
i am wondering, can we do the virtual generated column expansion in
the rewrite stage as is,
and wrap the expressions in PHVs if the virtual generated
columns come from the nullable side of an outer join.
I am looking at pullup_replace_vars_callback, but it seems not very
helpful to us.
From 2f9bd9ec737227b1b2dc52a4800d006bfa228003 Mon Sep 17 00:00:00 2001
From: jian he <[email-masked]>
Date: Thu, 13 Feb 2025 11:28:57 +0800
Subject: [PATCH v2 1/1] fix expand virtual generated column Var node
 varnullingrels field

To expand the virtual generated column Var node,
we need to copy the fields of the original virtual generated column Var node,
including varnullingrels, varlevelsup, varreturningtype, and varattno,
to the newly expanded Var node.

ReplaceVarsFromTargetList_callback didn't taken care of varnullingrels,
this patch fix this issue.

discussion: https://postgr.es/m/[email-masked]
---
 src/backend/rewrite/rewriteManip.c            | 62 +++++++++++++++++++
 src/include/rewrite/rewriteManip.h            |  3 +
 .../regress/expected/generated_virtual.out    | 45 ++++++++++++++
 src/test/regress/sql/generated_virtual.sql    | 11 ++++
 src/tools/pgindent/typedefs.list              |  1 +
 5 files changed, 122 insertions(+)

diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index a115b217c91..69701ce6ecd 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -884,6 +884,64 @@ IncrementVarSublevelsUp_rtable(List *rtable, int delta_sublevels_up,
 					   QTW_EXAMINE_RTES_BEFORE);
 }
 
+/*
+ * SetVarNullingrels - adjust Var nodes for a specified varnullingrels.
+ *
+ * Find all Var nodes referring to the specified varno in the given
+ * expression and set their varnullingrels to the specified value.
+ */
+typedef struct
+{
+	int			sublevels_up;
+	int			varno;
+	Bitmapset *varnullingrels;
+} SetVarNullingrels_context;
+
+static bool
+SetVarNullingrels_walker(Node *node,
+						 SetVarNullingrels_context *context)
+{
+	if (node == NULL)
+		return false;
+
+	if (IsA(node, Var))
+	{
+		Var		   *var = (Var *) node;
+
+		if (var->varlevelsup == context->sublevels_up &&
+			var->varno == context->varno)
+			var->varnullingrels = bms_union(context->varnullingrels,
+											var->varnullingrels);
+
+		return false;
+	}
+
+	if (IsA(node, Query))
+	{
+		/* Recurse into subselects */
+		bool		result;
+
+		context->sublevels_up++;
+		result = query_tree_walker((Query *) node, SetVarNullingrels_walker,
+								   context, 0);
+		context->sublevels_up--;
+		return result;
+	}
+	return expression_tree_walker(node, SetVarNullingrels_walker, context);
+}
+void
+SetVarNullingrels(Node *node, int sublevels_up, int varno, Bitmapset *varnullingrels)
+{
+	SetVarNullingrels_context context;
+
+	context.sublevels_up = sublevels_up;
+	context.varno = varno;
+	context.varnullingrels = varnullingrels;
+
+	/* Expect to start with an expression */
+	SetVarNullingrels_walker(node, &context);
+}
+
 /*
  * SetVarReturningType - adjust Var nodes for a specified varreturningtype.
  *
@@ -1832,6 +1890,10 @@ ReplaceVarsFromTargetList_callback(Var *var,
 		if (var->varlevelsup > 0)
 			IncrementVarSublevelsUp((Node *) newnode, var->varlevelsup, 0);
 
+							  var->varno,
index 512823033b9..e869f651fc8 100644
@@ -48,6 +48,9 @@ extern void ChangeVarNodes(Node *node, int rt_index, int new_index,
+							  int varno,
 
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 35638812be9..56e7e6fd6b0 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -181,6 +181,51 @@ SELECT * FROM gtestx, gtest1 WHERE gtestx.y = gtest1.a;
  22 | 2 | 2 | 4
 (2 rows)
 
+SELECT t.x FROM gtestx t LEFT JOIN gtest1 vt on t.x = vt.a WHERE coalesce(vt.b, 1) = 1 OR t.x IS NULL;
+ x  
+----
+ 11
+ 22
+ 33
+(3 rows)
index 34870813910..d76439ca993 100644
@@ -80,6 +80,17 @@ DELETE FROM gtest1 WHERE a = 2000000000;
+SELECT t.x FROM gtestx t LEFT JOIN gtest1 vt on t.x = vt.a WHERE coalesce(vt.b, 1) = 1 OR t.x IS NULL;
+
 DROP TABLE gtestx;
 
 -- test UPDATE/DELETE quals
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index b6c170ac249..6cb39ac76c5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2633,6 +2633,7 @@ SetOperation
 SetOperationStmt
 SetQuantifier
 SetToDefault
+SetVarNullingrels_context
 SetVarReturningType_context
 SetupWorkerPtrType
 ShDependObjectInfo
-- 
2.34.1

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 14 Feb 2025 11:59:48 +0100

> columns come from the nullable side of an outer join.

PlaceHolderVar looks like a fitting mechanism for this.  But it's so far 
a planner node, so it might take some additional consideration if we 
want to expand where it's used.

Maybe a short-term fix would be to error out if we find ourselves about 
to expand a Var with varnullingrels != NULL.  That would mean you 
couldn't use a virtual generated column on the nullable output side of 
an outer join, which is annoying but not fatal, and we could fix it 
incrementally later.

---

From: Dean Rasheed <[email-masked]>
Date: Sat, 15 Feb 2025 12:37:45 +0000

> want to expand where it's used.

It seems pretty baked into how PHVs work that they should only be
added by the planner, so I think that I agree with Richard -- virtual
generated columns probably have to be expanded in the planner rather
than the rewriter.

> an outer join, which is annoying but not fatal, and we could fix it

I think that would be rather a sad limitation to have. It would be
nice to have this fully working for the next release.

Attached is a rough patch that moves the expansion of virtual
generated columns to the planner. It needs a lot more testing (and
some regression tests), but it does seem to fix all the issues
mentioned in this thread.

Regards,
Dean
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 7b1a8a0..041c152
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -712,6 +712,13 @@ subquery_planner(PlannerGlobal *glob, Qu
 	transform_MERGE_to_join(parse);
 
 	/*
+	 * Expand any virtual generated columns.  Note that this step does not
+	 * descend into subqueries; if we pull up any subqueries below, their
+	 * virtual generated columns are expanded just before pulling them up.
+	 */
+	parse = root->parse = expand_virtual_generated_columns(root, parse);
+
+	/*
 	 * If the FROM clause is empty, replace it with a dummy RTE_RESULT RTE, so
 	 * that we don't need so many special cases to deal with that situation.
 	 */
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
new file mode 100644
index 5d9225e..95ef3f4
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -4,6 +4,8 @@
  *	  Planner preprocessing for subqueries and join tree manipulation.
  *
  * NOTE: the intended sequence for invoking these operations is
+ *		transform_MERGE_to_join
+ *		expand_virtual_generated_columns
  *		replace_empty_jointree
  *		pull_up_sublinks
  *		preprocess_function_rtes
@@ -25,6 +27,7 @@
  */
 #include "postgres.h"
 
+#include "access/table.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "miscadmin.h"
@@ -39,8 +42,25 @@
 #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 expand_generated_table_context
+{
+	int			num_attrs;		/* number of table columns */
+	List	   *tlist;			/* virtual generated column replacements */
+	Node	  **eg_cache;		/* cache for results with PHVs */
+} expand_generated_table_context;
 
+typedef struct expand_generated_context
+{
+	PlannerInfo *root;
+	Query	   *parse;			/* query being rewritten */
+	int			sublevels_up;	/* (current) nesting depth */
+	expand_generated_table_context *tbl_contexts;	/* per RTE of query */
+} expand_generated_context;
 
 typedef struct nullingrel_info
 {
@@ -87,6 +107,8 @@ typedef struct reduce_outer_joins_partia
 	Relids		unreduced_side; /* relids in its still-nullable side */
 } reduce_outer_joins_partial_state;
 
+static Node *expand_virtual_generated_columns_callback(Node *node,
+													   expand_generated_context *context);
 static Node *pull_up_sublinks_jointree_recurse(PlannerInfo *root, Node *jtnode,
 											   Relids *relids);
 static Node *pull_up_sublinks_qual_recurse(PlannerInfo *root, Node *node,
@@ -378,6 +400,227 @@ transform_MERGE_to_join(Query *parse)
 }
 
 /*
+ * Expand all virtual generated column references in a query.
+ *
+ * Note that this only processes virtual generated columns in relation RTEs
+ * in the query's rtable; it does not descend into subqueries.
+ */
+Query *
+expand_virtual_generated_columns(PlannerInfo *root, Query *parse)
+{
+	expand_generated_context egcontext;
+	int			rt_index;
+
+	egcontext.root = root;
+	egcontext.parse = parse;
+	egcontext.sublevels_up = 0;
+	egcontext.tbl_contexts = NULL;
+
+	/* Scan the range table for relations with virtual generated columns */
+	rt_index = 0;
+	foreach_node(RangeTblEntry, rte, parse->rtable)
+	{
+		Relation	rel;
+		int			num_attrs;
+		List	   *tlist;
+
+		++rt_index;
+
+		/* Only normal relations can have virtual generated columns */
+		if (rte->rtekind != RTE_RELATION)
+			continue;
+
+		/*
+		 * Look for virtual generated columns.  We assume that previous code
+		 * already acquired a lock on the table, so we need no lock here.
+		 */
+		rel = table_open(rte->relid, NoLock);
+		num_attrs = RelationGetDescr(rel)->natts;
+		tlist = get_virtual_generated_columns(rel, rt_index);
+		table_close(rel, NoLock);
+
+		if (tlist)
+		{
+			expand_generated_table_context *tctx;
+
+			/*
+			 * Record details of this table's virtual generated columns. The
+			 * first time through, build the per-table context array.
+			 */
+			if (egcontext.tbl_contexts == NULL)
+				egcontext.tbl_contexts = palloc0(list_length(parse->rtable) *
+												 sizeof(expand_generated_table_context));
+
+			tctx = &egcontext.tbl_contexts[rt_index - 1];
+			tctx->num_attrs = num_attrs;
+			tctx->tlist = tlist;
+			/* PHV cache for each attr, plus wholerow (attrnum = 0) */
+			tctx->eg_cache = palloc0((num_attrs + 1) * sizeof(Node *));
+		}
+	}
+
+	/*
+	 * If any relations had virtual generated columns, perform the necessary
+	 * replacements.
+	 */
+	if (egcontext.tbl_contexts != NULL)
+		parse = query_tree_mutator(parse,
+								   expand_virtual_generated_columns_callback,
+								   &egcontext,
+								   0);
+
+	return parse;
+}
+
+static Node *
+expand_virtual_generated_columns_callback(Node *node,
+										  expand_generated_context *context)
+{
+	if (node == NULL)
+		return NULL;
+	else if (IsA(node, Var))
+	{
+		Query	   *parse = context->parse;
+		Var		   *var = (Var *) node;
+
+		/*
+		 * If the Var is from a relation with virtual generated columns, then
+		 * replace it with the appropriate expression, if necessary.
+		 */
+		if (var->varlevelsup == context->sublevels_up &&
+			var->varno > 0 &&
+			var->varno <= list_length(parse->rtable) &&
+			context->tbl_contexts[var->varno - 1].tlist)
+		{
+			expand_generated_table_context *tctx;
+			Node	   *newnode;
+
+			tctx = &context->tbl_contexts[var->varno - 1];
+
+			/*
+			 * If the Var has nonempty varnullingrels, the replacement
+			 * expression (if any) is wrapped in a PlaceHolderVar, unless it
+			 * is simply another Var.  These are cached to avoid generating
+			 * identical PHVs with different IDs, which would lead to
+			 * duplicate evaluations at runtime.  See similar code in
+			 * pullup_replace_vars_callback().
+			 *
+			 * The cached items have phlevelsup = 0 and phnullingrels = NULL,
+			 * so we need to copy and adjust them.
+			 */
+			if (var->varnullingrels != NULL &&
+				var->varattno >= InvalidAttrNumber &&
+				var->varattno <= tctx->num_attrs &&
+				tctx->eg_cache[var->varattno] != NULL)
+			{
+				/* Copy the cached item and adjust its varlevelsup */
+				newnode = copyObject(tctx->eg_cache[var->varattno]);
+				if (var->varlevelsup > 0)
+					IncrementVarSublevelsUp(newnode, var->varlevelsup, 0);
+			}
+			else
+			{
+				/*
+				 * Generate the replacement expression. This takes care of
+				 * expanding wholerow references, as well as adjusting
+				 * varlevelsup and varreturningtype.
+				 */
+				newnode = ReplaceVarFromTargetList(var,
+												   rt_fetch(var->varno,
+															parse->rtable),
+												   tctx->tlist,
+												   parse->resultRelation,
+												   REPLACEVARS_CHANGE_VARNO,
+												   var->varno);
+
+				/* Insert PlaceHolderVar if needed */
+				if (var->varnullingrels != NULL && !IsA(newnode, Var))
+				{
+					/* XXX: Consider not wrapping every expression */
+					newnode = (Node *)
+						make_placeholder_expr(context->root,
+											  (Expr *) newnode,
+											  bms_make_singleton(var->varno));
+
+					/*
+					 * Adjust the PlaceHolderVar's phlevelsup (the wrapped
+					 * expression itself is already correct at this point).
+					 */
+					((PlaceHolderVar *) newnode)->phlevelsup = var->varlevelsup;
+
+					/*
+					 * Cache it if possible (ie, if the attno is in range,
+					 * which it probably always should be), ensuring that the
+					 * cached item has phlevelsup = 0.
+					 */
+					if (var->varattno >= InvalidAttrNumber &&
+						var->varattno <= tctx->num_attrs)
+					{
+						Node	   *cachenode = copyObject(newnode);
+
+						if (var->varlevelsup > 0)
+							IncrementVarSublevelsUp(cachenode,
+													-((int) var->varlevelsup),
+													0);
+
+						tctx->eg_cache[var->varattno] = cachenode;
+					}
+				}
+			}
+
+			/* Propagate any varnullingrels into the replacement expression */
+			if (var->varnullingrels != NULL)
+			{
+				if (IsA(newnode, Var))
+				{
+					Var		   *newvar = (Var *) newnode;
+
+					Assert(newvar->varlevelsup == var->varlevelsup);
+					newvar->varnullingrels = bms_add_members(newvar->varnullingrels,
+															 var->varnullingrels);
+				}
+				else if (IsA(newnode, PlaceHolderVar))
+				{
+					PlaceHolderVar *newphv = (PlaceHolderVar *) newnode;
+
+					Assert(newphv->phlevelsup == var->varlevelsup);
+					newphv->phnullingrels = bms_add_members(newphv->phnullingrels,
+															var->varnullingrels);
+				}
+				else
+				{
+					/*
+					 * Currently, the code above wraps all non-Var expressions
+					 * in PlaceHolderVars, so we should never see anything
+					 * else here.  If that changes, this will need code
+					 * similar to that in pullup_replace_vars_callback().
+					 */
+					elog(ERROR, "unexpected expression for virtual generated column");
+				}
+			}
+
+			return newnode;
+		}
+	}
+	else if (IsA(node, Query))
+	{
+		Query	   *newnode;
+
+		context->sublevels_up++;
+		newnode = query_tree_mutator((Query *) node,
+									 expand_virtual_generated_columns_callback,
+									 context,
+									 0);
+		context->sublevels_up--;
+
+		return (Node *) newnode;
+	}
+	return expression_tree_mutator(node,
+								   expand_virtual_generated_columns_callback,
+								   context);
+}
+
+/*
  * replace_empty_jointree
  *		If the Query's jointree is empty, replace it with a dummy RTE_RESULT
  *		relation.
@@ -1179,6 +1422,13 @@ pull_up_simple_subquery(PlannerInfo *roo
 	Assert(subquery->cteList == NIL);
 
 	/*
+	 * Expand any virtual generated columns in the subquery, before we pull it
+	 * up (this has already been done for the parent query).
+	 */
+	subquery = subroot->parse = expand_virtual_generated_columns(subroot,
+																 subquery);
+
+	/*
 	 * If the FROM clause is empty, replace it with a dummy RTE_RESULT RTE, so
 	 * that we don't need so many special cases to deal with that situation.
 	 */
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index e996bdc..ecc695f
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -96,8 +96,6 @@ static List *matchLocks(CmdType event, R
 						int varno, Query *parsetree, bool *hasUpdate);
 static Query *fireRIRrules(Query *parsetree, List *activeRIRs);
 static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist);
-static Node *expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
-											   RangeTblEntry *rte, int result_relation);

 /*
@@ -2190,10 +2188,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 +2201,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 +2293,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);
 	}
 
@@ -4425,31 +4408,20 @@ RewriteQuery(Query *parsetree, List *rew

 /*
- * Expand virtual generated columns
- *
- * If the table contains virtual generated columns, build a target list
- * containing the expanded expressions and use ReplaceVarsFromTargetList() to
- * do the replacements.
- *
- * Vars matching rt_index at the current query level are replaced by the
- * virtual generated column expressions from rel, if there are any.
+ * Get the virtual generated columns of a relation.
  *
- * The caller must also provide rte, the RTE describing the target relation,
- * in order to handle any whole-row Vars referencing the target, and
- * result_relation, the index of the result relation, if this is part of an
- * INSERT/UPDATE/DELETE/MERGE query.
+ * The returned list is in the form of a targetlist (of TargetEntry nodes)
+ * suitable for use by ReplaceVarFromTargetList/ReplaceVarsFromTargetList to
+ * expand references to virtual generated columns.
  */
-static Node *
-expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
-								  RangeTblEntry *rte, int result_relation)
+List *
+get_virtual_generated_columns(Relation rel, int rt_index)
 {
-	TupleDesc	tupdesc;
+	TupleDesc	tupdesc = RelationGetDescr(rel);
+	List	   *tlist = NIL;
 
-	tupdesc = RelationGetDescr(rel);
 	if (tupdesc->constr && tupdesc->constr->has_generated_virtual)
 	{
-		List	   *tlist = NIL;
-
 		for (int i = 0; i < tupdesc->natts; i++)
 		{
 			Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
@@ -4491,14 +4463,8 @@ expand_generated_columns_internal(Node *
 		}
 
 		Assert(list_length(tlist) > 0);
-
-		node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, tlist,
-										 result_relation,
-										 REPLACEVARS_CHANGE_VARNO, rt_index,
-										 NULL);
 	}
-
-	return node;
+	return tlist;
 }
 
 /*
@@ -4515,6 +4481,7 @@ expand_generated_columns_in_expr(Node *n
 	if (tupdesc->constr && tupdesc->constr->has_generated_virtual)
 	{
 		RangeTblEntry *rte;
+		List	   *tlist;
 
 		rte = makeNode(RangeTblEntry);
 		/* eref needs to be set, but the actual name doesn't matter */
@@ -4522,7 +4489,11 @@ expand_generated_columns_in_expr(Node *n
 		rte->rtekind = RTE_RELATION;
 		rte->relid = RelationGetRelid(rel);
 
-		node = expand_generated_columns_internal(node, rel, rt_index, rte, 0);
+		tlist = get_virtual_generated_columns(rel, rt_index);
+
+		node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, tlist, 0,
+										 REPLACEVARS_CHANGE_VARNO, rt_index,
+										 NULL);
 	}
 
 	return node;
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..9c93643
--- a/src/include/optimizer/prep.h
+++ b/src/include/optimizer/prep.h
@@ -22,6 +22,7 @@
  * prototypes for prepjointree.c
  */
 extern void transform_MERGE_to_join(Query *parse);
+extern Query *expand_virtual_generated_columns(PlannerInfo *root, Query *parse);
 extern void replace_empty_jointree(Query *parse);
 extern void pull_up_sublinks(PlannerInfo *root);
 extern void preprocess_function_rtes(PlannerInfo *root);
diff --git a/src/include/rewrite/rewriteHandler.h b/src/include/rewrite/rewriteHandler.h
new file mode 100644
index 88fe13c..21876cf
--- a/src/include/rewrite/rewriteHandler.h
+++ b/src/include/rewrite/rewriteHandler.h
@@ -38,6 +38,7 @@ extern void error_view_not_updatable(Rel
 									 List *mergeActionList,
 									 const char *detail);
 
+extern List *get_virtual_generated_columns(Relation rel, int rt_index);
 extern Node *expand_generated_columns_in_expr(Node *node, Relation rel, int rt_index);
 
 #endif							/* REWRITEHANDLER_H */
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,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
new file mode 100644
index b6c170a..4d0fcab
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3484,6 +3484,8 @@ eval_const_expressions_context
 exec_thread_arg
 execution_state
 exit_function
+expand_generated_context
+expand_generated_table_context
 explain_get_index_name_hook_type
 f_smgr
 fasthash_state

---

From: jian he <[email-masked]>
Date: Mon, 17 Feb 2025 14:48:18 +0800

> nice to have this fully working for the next release.
error out seems not that hard, IMHO.
i think, we still have time to figure out how to make it fully working in v18.

> mentioned in this thread.
I will review it later.
In the meantime,
I also came up with my patch (including tests) that solves all the issues.

---

From: Richard Guo <[email-masked]>
Date: Tue, 18 Feb 2025 19:09:17 +0900

> nice to have this fully working for the next release.

Besides being a limitation, this approach doesn't address all the
issues with incorrect results.  In some cases, PHVs are needed to
isolate subexpressions, even when varnullingrels != NULL.  As an
example, please consider

create table t (a int primary key, b int generated always as (10 + 10));
insert into t values (1);
insert into t values (2);

# select a, b from t group by grouping sets (a, b) having b = 20;
 a | b
---+----
 2 |
 1 |
   | 20
(3 rows)

This result set is incorrect.  The first two rows, where b is NULL,
should not be included in the result set.

> mentioned in this thread.

Yeah, I believe this is the right way to go: virtual generated columns
should be expanded in the planner, rather than in the rewriter.

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.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Tue, 18 Feb 2025 22:12:31 +0900

> pullup_replace_vars_callback.

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.

Thanks
Richard

---

From: Dean Rasheed <[email-masked]>
Date: Wed, 19 Feb 2025 01:42:38 +0000

> > 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.

> 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"
 
@@ -39,7 +41,9 @@

 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;
 
-		/*
 
 		rel = table_open(rte->relid, NoLock);
@@ -2300,16 +2295,6 @@ fireRIRrules(Query *parsetree, List *act
 		if (hasSubLinks)
 			parsetree->hasSubLinks = true;
 
-		/*

 
 		/*

 

 
 			/* 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);

---

From: Dean Rasheed <[email-masked]>
Date: Wed, 19 Feb 2025 15:25:05 +0000

> have just one place to maintain. Attached is an updated patch doing

I've been doing some more testing of this, and attached is another
update, improving a few comments and adding regression tests based on
the cases discussed so far here.

One of the new regression tests fails, which actually appears to be a
pre-existing grouping sets bug, due to the fact that only non-Vars are
wrapped in PHVs. This bug can be triggered without virtual generated
columns:

CREATE TABLE t (a int, b int);
INSERT INTO t VALUES (1, 1);

SELECT * FROM (SELECT a, a AS b FROM t) AS vt
GROUP BY GROUPING SETS (a, b)
HAVING b = 1;

 a | b
---+---
 1 |
(1 row)

whereas the result should be

 a | b
---+---
   |  1
(1 row)

For reference, this code dates back to 90947674fc.

Regards,
Dean
From 057d8a8f63d7b6b5d8efb5d4f519b8557e264a36 Mon Sep 17 00:00:00 2001
From: Dean Rasheed <[email-masked]>
Date: Wed, 19 Feb 2025 10:30:33 +0000
Subject: [PATCH v4] Expand virtual generated columns in the planner.

Commit 83ea6c54025 added support for virtual generated columns, which
were expanded in the rewriter, in a similar way to view columns.
However, that does not propagate varnullingrels onto the replacement
expressions, which is OK for view columns because they are expanded
inside a subquery, but it is not OK for virtual generated columns
which are expanded directly into the main query and so must be
correctly marked. This lead to various problems with outer join
queries, including "wrong varnullingrels" errors, incorrect results,
and improper outer-join removal.

To fix this, expanded virtual generated columns must be properly
marked, based on the original Var's varnullingrels, and if necessary,
wrapped in PlaceHolderVars. Since PlaceHolderVar is a planner node,
this really has to be done in the planner. To avoid a lot of code
duplication, reuse the planner's subquery-pullup code for this.

Since this now means that the subquery-pullup code may be dealing with
Vars referring to the result relation rather than a subquery, it must
also be taught to handle Vars with non-default varreturningtype
(OLD/NEW references in the RETURNING list), which previously only the
rewriter had to worry about. To avoid even more code duplication
between this code and the rewriter, arrange for pullup_replace_vars()
to reuse ReplaceVarsFromTargetList()'s mutator function. This
eliminates a lot of existing duplicated code between the rewriter and
the planner, and avoids adding even more.
---
 src/backend/optimizer/plan/planner.c      |   6 +
 src/backend/optimizer/prep/prepjointree.c | 262 +++++++++++++++-------
 src/backend/rewrite/rewriteHandler.c      |  23 +-
 src/backend/rewrite/rewriteManip.c        |  50 ++++-
 src/include/optimizer/prep.h              |   1 +
 src/include/rewrite/rewriteManip.h        |   7 +
 src/test/regress/expected/join.out        |  66 ++++++
 src/test/regress/sql/join.sql             |  43 ++++
 8 files changed, 351 insertions(+), 107 deletions(-)

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 7b1a8a0a9f1..6beb58e8f04 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -749,6 +749,12 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
 	if (parse->setOperations)
 		flatten_simple_union_all(root);
 
+	/*
+	 * Scan the rangetable for relations with virtual generated columns, and
+	 * expand them.
+	 */
index 5d9225e9909..8fdca35d087 100644

 typedef struct nullingrel_info
@@ -57,6 +61,7 @@ typedef struct pullup_replace_vars_context
 {
 	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 *root, Node *jtnode, RangeTblEntry *rte,
 	 */
 	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, Node *jtnode, RangeTblEntry *rte)
 	}
 	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 *root, Node *jtnode,
 													  1,	/* resno */
 													  NULL, /* resname */
 													  false));	/* resjunk */
+	rvcontext.result_relation = 0;
 	rvcontext.target_rte = rte;
 
 	/*
@@ -2490,6 +2498,10 @@ pullup_replace_vars_callback(Var *var,
 	bool		need_phv;
 	Node	   *newnode;
 
 	 * We need a PlaceHolderVar if the Var-to-be-replaced has nonempty
 	 * varnullingrels (unless we find below that the replacement expression is
+		 * wholerow references, as well as adjusting varlevelsup and dealing
+		 * with non-default varreturningtype.

-			if (newnode && IsA(newnode, Var) &&
-				((Var *) newnode)->varlevelsup == 0)
+			if (varattno == InvalidAttrNumber)
+			{
+				/*
+				 * A wholerow Var will have been 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;
 
 				/*

 
 
@@ -2947,6 +2924,135 @@ flatten_simple_union_all(PlannerInfo *root)
 }

+/*
+ * This scans the rangetable for relations with virtual generated columns, and
+ * replaces all Var nodes in the query that refer to such columns with the
+ * appropriate expressions.  Note that this happens after subquery pullup, and
+ * it does not recurse into any remaining subqueries; that is taken care of
+ * when those subqueries are planned.
+ * Returns a modified copy of the query tree, if any relations with virtual
+ * generated columns are present.
index e996bdc0d21..7a39abd4d86 100644
@@ -2190,10 +2190,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
@@ -2207,11 +2203,10 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
 
 		++rt_index;
 
-		/*
 
 		rel = table_open(rte->relid, NoLock);
@@ -2300,16 +2295,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
 		if (hasSubLinks)
 			parsetree->hasSubLinks = true;
 
-		/*
 
index 9433548d279..e81624f9b09 100644
@@ -1814,6 +1814,23 @@ ReplaceVarsFromTargetList_callback(Var *var,
 
 	if (var->varattno == InvalidAttrNumber)
@@ -1822,6 +1839,7 @@ ReplaceVarsFromTargetList_callback(Var *var,
 		RowExpr    *rowexpr;
 		List	   *colnames;
 		List	   *fields;
+		ListCell   *lc;
 
 		/*
@@ -1833,15 +1851,27 @@ ReplaceVarsFromTargetList_callback(Var *var,
@@ -1863,12 +1893,12 @@ ReplaceVarsFromTargetList_callback(Var *var,
 	}

@@ -1876,7 +1906,7 @@ ReplaceVarsFromTargetList_callback(Var *var,

@@ -1932,15 +1962,15 @@ ReplaceVarsFromTargetList_callback(Var *var,

index 0ae57ec24a4..6bc75ac9f38 100644
@@ -27,6 +27,7 @@ extern void pull_up_sublinks(PlannerInfo *root);
index 5ec475c63e9..8ca0face062 100644
@@ -89,6 +89,13 @@ extern Node *map_variable_attnos(Node *node,
 
+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,
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index a57bb18c24f..e787792ece1 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -3514,6 +3514,72 @@ order by c.name;
  C    |      |       |      
 (3 rows)
 
+rollback;
+--
+-- test NULL behavior of virtual generated columns
+--
+begin;
+create temp table t (
+     a int primary key,
+     b int generated always as (1 + 1),
+     c int generated always as (a),
+     d int generated always as (a * 10),
+     e int generated always as (coalesce(a, 100))
+);
+insert into t values (1), (2);
+-- test propagtion of varnullingrels
+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),
+       sum(t2.e) over (partition by t2.a)
+from t as t1 left join t as t2 on (t1.a = t2.a)
+order by t1.a;
+ sum | sum | sum | sum 
+-----+-----+-----+-----
+   2 |   1 |  10 |   1
+   2 |   2 |  20 |   2
+(2 rows)
+
+select t1.*, t2.*
+from t as t1 left join t as t2 on false
+order by t1.a;
+ a | b | c | d  | e | a | b | c | d | e 
+---+---+---+----+---+---+---+---+---+---
+ 1 | 2 | 1 | 10 | 1 |   |   |   |   |  
+ 2 | 2 | 2 | 20 | 2 |   |   |   |   |  
+(2 rows)
+
+explain (costs off)
+select t1.a
+from t as t1 left join t as t2 on (t1.a = t2.a)
+where coalesce(t2.d, 1) = 1;
+                QUERY PLAN                
+------------------------------------------
+ Hash Left Join
+   Hash Cond: (t1.a = t2.a)
+   Filter: (COALESCE((t2.a * 10), 1) = 1)
+   ->  Seq Scan on t t1
+   ->  Hash
+         ->  Seq Scan on t t2
+(6 rows)
+
+-- grouping sets requires PlaceHolderVar for non-Var clauses
+select * from t
+group by grouping sets (a, b, c, d, e)
+having b = 2;
+ a | b | c | d | e 
+---+---+---+---+---
+   | 2 |   |   |  
+(1 row)
+
+select * from t
+group by grouping sets (a, b, c, d, e)
+having c = 2;
+ a | b | c | d | e 
+---+---+---+---+---
+   |   | 2 |   |  
+(1 row)
+
 rollback;
 --
 -- test incorrect handling of placeholders that only appear in targetlists,
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index c29d13b9fed..828cbdfd9db 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1044,6 +1044,49 @@ order by c.name;
 
 rollback;
 
+--
+-- test NULL behavior of virtual generated columns
+--
+begin;
+
+create temp table t (
+     a int primary key,
+     b int generated always as (1 + 1),
+     c int generated always as (a),
+     d int generated always as (a * 10),
+     e int generated always as (coalesce(a, 100))
+);
+
+insert into t values (1), (2);
+
+-- test propagtion of varnullingrels
+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),
+       sum(t2.e) over (partition by t2.a)
+from t as t1 left join t as t2 on (t1.a = t2.a)
+order by t1.a;
+
+select t1.*, t2.*
+from t as t1 left join t as t2 on false
+order by t1.a;
+
+explain (costs off)
+select t1.a
+from t as t1 left join t as t2 on (t1.a = t2.a)
+where coalesce(t2.d, 1) = 1;
+
+-- grouping sets requires PlaceHolderVar for non-Var clauses
+select * from t
+group by grouping sets (a, b, c, d, e)
+having b = 2;
+
+select * from t
+group by grouping sets (a, b, c, d, e)
+having c = 2;
+
+rollback;
+
 --
 -- test incorrect handling of placeholders that only appear in targetlists,
 -- per bug #6154
-- 
2.43.0

---

From: jian he <[email-masked]>
Date: Thu, 20 Feb 2025 12:57:38 +0800

> the cases discussed so far here.

hi.
patch v4, seems still not bullet-proof.

create table t (
     a int primary key,
     b int generated always as (1 + 1),
     c int generated always as (a),
     d int generated always as (a * 10),
     e int generated always as (coalesce(a, 100))
);
insert into t values (1), (2);
select a,c from t group by grouping sets (a,c) having c = 2;
a | c
---+---
 2 |

we should expect
 a | c
---+---
   | 2

---

From: jian he <[email-masked]>
Date: Fri, 21 Feb 2025 12:43:41 +0800

> For reference, this code dates back to 90947674fc.

sorry for the noise.
i misunderstood your message.
you’ve already mentioned this problem.

in struct pullup_replace_vars_context
adding a field (bool wrap_vars) and setting it appropriately in
function pullup_replace_vars_callback
seems to solve this problem.

---

From: Richard Guo <[email-masked]>
Date: Fri, 21 Feb 2025 15:16:35 +0900

On Thu, Feb 20, 2025 at 12:25 AM Dean Rasheed <[email-masked]> wrote:
> > have just one place to maintain.

Yeah, it's annoying that the two replace_rte_variables callbacks have
so much code duplication.  I think it's a win to make them share
common code.  What do you think about making this refactor a separate
patch, as it doesn't seem directly related to the bug fix here?

> the cases discussed so far here.

Hmm, there are some issues with v4 as far as I can see.

* In pullup_replace_vars_callback, the varlevelsup of the newnode is
adjusted before its nullingrels is updated.  This can cause problems.
If the newnode is not a Var/PHV, we adjust its nullingrels with
add_nulling_relids, and this function only works for level-zero vars.
As a result, we may fail to put the varnullingrels into the
expression.

I think we should insist that ReplaceVarFromTargetList generates the
replacement expression with varlevelsup = 0, and that the caller is
responsible for adjusting the varlevelsup if needed.  This may need
some changes to ReplaceVarsFromTargetList_callback too.

* When expanding whole-tuple references, it is possible that some
fields are expanded as Consts rather than Vars, considering dropped
columns.  I think we need to check for this when generating the fields
for a RowExpr.

* 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).

* Not an issue but I think that maybe we can share some common code
between expand_virtual_generated_columns and
expand_generated_columns_internal on how we build the generation
expressions for a virtual generated column.

I've worked on these issues and attached are the updated patches.
0001 expands virtual generated columns in the planner.  0002 refactors
the code to eliminate code duplication in the replace_rte_variables
callback functions.

> wrapped in PHVs. This bug can be triggered without virtual generated

Interesting. I'll take a look at this issue.

Thanks
Richard

---

From: Dean Rasheed <[email-masked]>
Date: Fri, 21 Feb 2025 17:35:44 +0000

> patch, as it doesn't seem directly related to the bug fix here?

OK. Makes sense.

> some changes to ReplaceVarsFromTargetList_callback too.

Ah, nice catch. Yes, that makes sense.

> columns.  I think we need to check for this when generating the fields

Yes, good point.

> 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.

> expressions for a virtual generated column.

Agreed. I had planned to do that, but ran out of steam.

> the code to eliminate code duplication in the replace_rte_variables

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. This is only a minor point,
because I don't think we ever currently need to wrap a newnode Var due
to differing varlevelsup, so all that was happening was that it was
wrapping when it didn't need to, which is actually harmless aside from
a small runtime performance hit.

Given that we're moving this part of expanding virtual generated
columns to the planner, I wonder if we should also move the other bits
(build_generation_expression and expand_generated_columns_in_expr)
too, so that they're all together. That could be a follow-on patch.

Regards,
Dean
From c7b65fa591ea2d7bd7bacf823f20650915fff089 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email-masked]>
Date: Fri, 21 Feb 2025 09:46:59 +0900
Subject: [PATCH v6 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          |   7 +
 src/backend/optimizer/prep/prepjointree.c     | 186 ++++++++++++++++++
 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, 372 insertions(+), 46 deletions(-)

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 7b1a8a0a9f1..201487eaf42 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -717,6 +717,13 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
 	 */
 	replace_empty_jointree(parse);
 
+	/*
+	 * 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.
+	 */
+	parse = root->parse = expand_virtual_generated_columns(root);
+
 	/*
 	 * Look for ANY and EXISTS SubLinks in WHERE and JOIN/ON clauses, and try
 	 * to transform them into joins.  Note that this step does not descend
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 5d9225e9909..3125567846f 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -5,6 +5,7 @@
  *
  * NOTE: the intended sequence for invoking these operations is
  *		replace_empty_jointree
+ *		expand_virtual_generated_columns
  *		pull_up_sublinks
  *		preprocess_function_rtes
  *		pull_up_subqueries
@@ -25,6 +26,7 @@
  */
 #include "postgres.h"

 typedef struct nullingrel_info
@@ -57,6 +61,7 @@ typedef struct pullup_replace_vars_context
 {
 	PlannerInfo *root;
 	List	   *targetlist;		/* tlist of subquery being pulled up */
+	int			result_relation;
 	RangeTblEntry *target_rte;	/* RTE of subquery */
 	Relids		relids;			/* relids within subquery, as numbered after
 								 * pullup (set only if target_rte->lateral) */
@@ -421,6 +426,128 @@ replace_empty_jointree(Query *parse)
 	parse->jointree->fromlist = list_make1(rtr);
 }
 
+/*
+ * 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
+ * appropriate expressions.  Note that we do not recurse into subqueries; that
+ * is taken care of when the subqueries are planned.
+ *
+ * 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.result_relation = parse->resultRelation;
+			rvcontext.target_rte = rte;
+			/* 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.  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_sublinks
  *		Attempt to pull up ANY and EXISTS SubLinks to be treated as
@@ -1184,6 +1311,13 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
 	 */
 	replace_empty_jointree(subquery);
 
+	/*
+	 * 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);
+
 	/*
 	 * Pull up any SubLinks within the subquery's quals, so that we don't
 	 * leave unoptimized SubLinks behind.
@@ -1273,6 +1407,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
 	 */
 	rvcontext.root = root;
 	rvcontext.targetlist = subquery->targetList;
+	rvcontext.result_relation = 0;
 	rvcontext.target_rte = rte;
 	if (rte->lateral)
 	{
@@ -1833,6 +1968,7 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
 	}
 	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;
@@ -1993,6 +2129,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 +2627,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 +2700,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 +2741,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 != 0)
+			{
+				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;
 
-		/*

-		/*
 
@@ -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..0a61cd126b7 100644
--- a/src/include/optimizer/prep.h
+++ b/src/include/optimizer/prep.h
@@ -23,6 +23,7 @@
  */
 extern void transform_MERGE_to_join(Query *parse);
 extern void replace_empty_jointree(Query *parse);
+extern Query *expand_virtual_generated_columns(PlannerInfo *root);
 extern void pull_up_sublinks(PlannerInfo *root);
 extern void preprocess_function_rtes(PlannerInfo *root);
 extern void pull_up_subqueries(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

From 4643a1832ba30c5c6ddee4643676045675f65e28 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email-masked]>
Date: Fri, 21 Feb 2025 14:01:56 +0900
Subject: [PATCH v6 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 3125567846f..367404735b8 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -2660,127 +2660,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 != 0)
-			{
-				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);

-			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
@@ -2926,7 +2837,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,
 	}

@@ -1876,7 +1918,8 @@ ReplaceVarsFromTargetList_callback(Var *var,
 
+				var->varlevelsup = 0;
 
@@ -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);

---

From: Richard Guo <[email-masked]>
Date: Sat, 22 Feb 2025 23:55:41 +0900

> 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

> 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

---

From: Richard Guo <[email-masked]>
Date: Sun, 23 Feb 2025 00:12:25 +0900

> longer, barring any objections.

Sign... I neglected to make the change in 0001 that a Var newnode
compares its varlevelsup with 0 when deciding to wrap it in a
ReturningExpr.  I made this change in 0002 though, so maybe we're good
here.  Still, I'll fix this later.

Thanks
Richard

---

From: jian he <[email-masked]>
Date: Mon, 24 Feb 2025 14:50:19 +0800

> here.  Still, I'll fix this later.
i also noticed this issue...

some minor comments about v7.

         * 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.
         *
        expandRTE(target_rte,
                  var->varno, 0 /* not varlevelsup */ ,
                  var->varreturningtype, var->location,
                  (var->vartype != RECORDOID),
                  &colnames, &fields);
the above comments should be put on top of ReplaceVarFromTargetList?
so people can easily catch it.
when using ReplaceVarFromTargetList,
they’ll be aware that they might need to call IncrementVarSublevelsUp
in the caller.

src/include/nodes/primnodes.h
 * ReturningExpr nodes never appear in a parsed Query --- they are only ever
 * inserted by the rewriter.
 */
typedef struct ReturningExpr
this comment needs to change?

on top of src/test/regress/sql/generated_virtual.sql, we have:
-- keep these tests aligned with generated_stored.sql

but gtest32 is only related to virtual generated column.
maybe add a comment saying gtest32 related tests do not
apply to stored generated column.

---

From: Richard Guo <[email-masked]>
Date: Mon, 24 Feb 2025 18:20:51 +0900

> some minor comments about v7.

Thanks for reviewing.

Here are the updated patches with revised comments and some tweaks to
the commit messages.  I plan to push them in one or two days.

Thanks
Richard

---

From: Dean Rasheed <[email-masked]>
Date: Mon, 24 Feb 2025 12:06:51 +0000

> the commit messages.  I plan to push them in one or two days.

LGTM.

Regards,
Dean

---

From: Richard Guo <[email-masked]>
Date: Tue, 25 Feb 2025 16:22:39 +0900

> LGTM.

Pushed.  Thanks all for working on this issue.

Thanks
Richard

---

From: Alexander Lakhin <[email-masked]>
Date: Fri, 16 May 2025 07:00:01 +0300

Hello Richard,

> ERROR:  unexpected virtual generated column reference

I've discovered yet another way to trigger that error:
create table vt (a int, b int generated always as (a * 2), c int);
insert into vt values(1);
alter table vt alter column c type bigint using b + c;

ERROR:  XX000: unexpected virtual generated column reference
LOCATION:  CheckVarSlotCompatibility, execExprInterp.c:2410

Shouldn't this be expected/supported?

Best regards,
Alexander Lakhin
Neon (https://neon.tech)

---

From: Richard Guo <[email-masked]>
Date: Fri, 16 May 2025 16:26:01 +0900

> LOCATION:  CheckVarSlotCompatibility, execExprInterp.c:2410

Thank you for the report.  It seems that we fail to expand references
to virtual generated columns in the NewColumnValues expression when
altering tables.  We might be able to fix it by:

@@ -6203,7 +6203,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
        NewColumnValue *ex = lfirst(l);

        /* expr already planned */
-       ex->exprstate = ExecInitExpr((Expr *) ex->expr, NULL);
+       ex->exprstate = ExecInitExpr((Expr *)
expand_generated_columns_in_expr((Node *) ex->expr, oldrel, 1), NULL);

Thanks
Richard

---

From: jian he <[email-masked]>
Date: Fri, 16 May 2025 16:34:25 +0800

> expand_generated_columns_in_expr((Node *) ex->expr, oldrel, 1), NULL);

we have used the USING expression in ATPrepAlterColumnType,
ATColumnChangeRequiresRewrite.
expanding it on ATPrepAlterColumnType seems to make more sense?

@@ -14467,7 +14467,7 @@ ATPrepAlterColumnType(List **wqueue,
                 */
                newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue));
                newval->attnum = attnum;
-               newval->expr = (Expr *) transform;
+               newval->expr = (Expr *)
expand_generated_columns_in_expr(transform, rel, 1);
                newval->is_generated = false;

---

From: Richard Guo <[email-masked]>
Date: Thu, 29 May 2025 12:06:33 +0900

>                 newval->is_generated = false;

Yeah, ATPrepAlterColumnType does seem like a better place.  But we
need to ensure that ATColumnChangeRequiresRewrite sees the expanded
version of the expression — your proposed change fails to do that.

Additionally, I think we also need to ensure that the virtual
generated columns are expanded before the expression is fed through
expression_planner, to ensure it can be successfully transformed into
an executable form.

Hence, the attached patch.

Thanks
Richard

---

From: jian he <[email-masked]>
Date: Mon, 2 Jun 2025 13:30:58 +0800

> Hence, the attached patch.

looks good to me.

---

From: Richard Guo <[email-masked]>
Date: Thu, 26 Jun 2025 12:38:56 +0900

> looks good to me.

Pushed.

Thanks
Richard