public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: Dean Rasheed <[email protected]>
Cc: Chao Li <[email protected]>
Cc: SATYANARAYANA NARLAPURAM <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: Re: Bug: Rule actions see wrong values for generated columns (NEW.gen reads OLD value)
Date: Mon, 20 Apr 2026 16:29:04 +0900
Message-ID: <CAMbWs49GX=-p_dD=2zQvHB4yfjwCttWSXg8Pz8xcZJJYAjcp1Q@mail.gmail.com> (raw)
In-Reply-To: <CAMbWs48s9EVTz__xq9s0nAGeh-Z918t4VoGD+pxCXOq0mnc_7Q@mail.gmail.com>
References: <CAHg+QDexGTmCZzx=73gXkY2ZADS6LRhpnU+-8Y_QmrdTS6yUhA@mail.gmail.com>
<CAMbWs4-SJ6B26ciFu_K_2M1ObDA=GirV87N1BKeAtSRmQGATUA@mail.gmail.com>
<[email protected]>
<CAMbWs48=QrLy8gZprTfCmEtzoaUku54JXvAcnoRSo+OC8vUpwA@mail.gmail.com>
<CAEZATCWYqyJJJcMNEj-LEvCSwog2H_HS61hTROZLKYuaiy5YTg@mail.gmail.com>
<CAMbWs49DcHqOtd_BzuFcr9Va+tD-2fNs8bGsbH_Zy2bXUTNM1g@mail.gmail.com>
<[email protected]>
<CAEZATCUacsV=ffircCgh+uYj7FBh4vy5mFKCAR7g=EkXuJZBng@mail.gmail.com>
<CAMbWs48s9EVTz__xq9s0nAGeh-Z918t4VoGD+pxCXOq0mnc_7Q@mail.gmail.com>
On Mon, Apr 20, 2026 at 2:25 PM Richard Guo <[email protected]> wrote:
> Yeah, this is a better approach. The change looks good to me.
>
> A nitpick: For the comment "The generated column expressions typically
> refer to new.attribute ...", maybe we can remove "typically", as
> generation expressions always refer to columns of the same relation.
I noticed a couple of issues after a further look.
1. The ReplaceVarsFromTargetList call on "gen_cols" fails to handle
hasSubLinks. This can cause error if targetList contains SubLinks:
update t set a = (select max(a) from t);
ERROR: replace_rte_variables inserted a SubLink, but has noplace to record it
2. The same bug fixed in this patch also exists in rule quals:
create table t (a int, b int generated always as (a *2));
insert into t values (1);
create rule rule_qual as on update to t where new.b > 100
do instead nothing;
update t set a = 100;
select * from t;
a | b
-----+-----
100 | 200
(1 row)
I think we should apply the same fix to CopyAndAddInvertedQual.
Attached v3 fixes them.
- Richard
Attachments:
[application/octet-stream] v3-0001-Fix-incorrect-NEW-references-to-generated-columns.patch (15.6K, 2-v3-0001-Fix-incorrect-NEW-references-to-generated-columns.patch)
download | inline diff:
From c095a378f82ec0bd570eb29feb4e3ca59907bc88 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Mon, 20 Apr 2026 15:35:54 +0900
Subject: [PATCH v3] Fix incorrect NEW references to generated columns in rule
rewriting
---
src/backend/rewrite/rewriteHandler.c | 145 +++++++++++++-----
.../regress/expected/generated_stored.out | 34 ++++
.../regress/expected/generated_virtual.out | 34 ++++
src/test/regress/sql/generated_stored.sql | 23 +++
src/test/regress/sql/generated_virtual.sql | 23 +++
5 files changed, 221 insertions(+), 38 deletions(-)
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 021c73f1b67..77b2c9bc622 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -97,8 +97,7 @@ static List *matchLocks(CmdType event, Relation relation,
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);
+static List *get_generated_columns(Relation rel, int rt_index, bool include_stored);
/*
@@ -642,12 +641,46 @@ rewriteRuleAction(Query *parsetree,
if ((event == CMD_INSERT || event == CMD_UPDATE) &&
sub_action->commandType != CMD_UTILITY)
{
+ RangeTblEntry *new_rte = rt_fetch(new_varno, sub_action->rtable);
+ Relation new_rel;
+ List *gen_cols;
+
+ /*
+ * The target list does not contain entries for generated columns
+ * (they are removed by rewriteTargetListIU), so we must build entries
+ * for them here, so that new.gen_col can be rewritten correctly.
+ */
+ new_rel = relation_open(new_rte->relid, NoLock);
+ gen_cols = get_generated_columns(new_rel, new_varno, true);
+ relation_close(new_rel, NoLock);
+
+ /*
+ * The generated column expressions refer to new.attribute, so they
+ * must be rewritten before they can be used as replacements.
+ */
+ gen_cols = (List *)
+ ReplaceVarsFromTargetList((Node *) gen_cols,
+ new_varno,
+ 0,
+ new_rte,
+ parsetree->targetList,
+ sub_action->resultRelation,
+ (event == CMD_UPDATE) ?
+ REPLACEVARS_CHANGE_VARNO :
+ REPLACEVARS_SUBSTITUTE_NULL,
+ current_varno,
+ &sub_action->hasSubLinks);
+
+ /*
+ * Now rewrite new.attribute in sub_action, using both the target list
+ * and the rewritten generated column expressions.
+ */
sub_action = (Query *)
ReplaceVarsFromTargetList((Node *) sub_action,
new_varno,
0,
- rt_fetch(new_varno, sub_action->rtable),
- parsetree->targetList,
+ new_rte,
+ list_concat(gen_cols, parsetree->targetList),
sub_action->resultRelation,
(event == CMD_UPDATE) ?
REPLACEVARS_CHANGE_VARNO :
@@ -2368,18 +2401,50 @@ CopyAndAddInvertedQual(Query *parsetree,
ChangeVarNodes(new_qual, PRS2_OLD_VARNO, rt_index, 0);
/* Fix references to NEW */
if (event == CMD_INSERT || event == CMD_UPDATE)
+ {
+ RangeTblEntry *rte = rt_fetch(rt_index, parsetree->rtable);
+ Relation rel;
+ List *gen_cols;
+
+ /*
+ * As in rewriteRuleAction, build entries for generated columns so
+ * that new.gen_col in the rule qualification can be rewritten
+ * correctly.
+ */
+ rel = relation_open(rte->relid, NoLock);
+ gen_cols = get_generated_columns(rel, PRS2_NEW_VARNO, true);
+ relation_close(rel, NoLock);
+
+ /*
+ * The generated column expressions refer to new.attribute, so they
+ * must be rewritten before they can be used as replacements.
+ */
+ gen_cols = (List *)
+ ReplaceVarsFromTargetList((Node *) gen_cols,
+ PRS2_NEW_VARNO,
+ 0,
+ rte,
+ parsetree->targetList,
+ parsetree->resultRelation,
+ (event == CMD_UPDATE) ?
+ REPLACEVARS_CHANGE_VARNO :
+ REPLACEVARS_SUBSTITUTE_NULL,
+ rt_index,
+ &parsetree->hasSubLinks);
+
new_qual = ReplaceVarsFromTargetList(new_qual,
PRS2_NEW_VARNO,
0,
- rt_fetch(rt_index,
- parsetree->rtable),
- parsetree->targetList,
+ rte,
+ list_concat(gen_cols,
+ parsetree->targetList),
parsetree->resultRelation,
(event == CMD_UPDATE) ?
REPLACEVARS_CHANGE_VARNO :
REPLACEVARS_SUBSTITUTE_NULL,
rt_index,
&parsetree->hasSubLinks);
+ }
/* And attach the fixed qual */
AddInvertedQual(parsetree, new_qual);
@@ -4530,36 +4595,31 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
/*
- * 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.
+ * Get a table's generated columns
*
- * Vars matching rt_index at the current query level are replaced by the
- * virtual generated column expressions from rel, if there are any.
+ * If include_stored is true, both stored and virtual generated columns are
+ * returned. Otherwise, only virtual generated columns are returned.
*
- * 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.
+ * Returns a list of TargetEntry, one for each generated column, containing
+ * the attribute numbers and generation expressions.
*/
-static Node *
-expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
- RangeTblEntry *rte, int result_relation)
+static List *
+get_generated_columns(Relation rel, int rt_index, bool include_stored)
{
+ List *gen_cols = NIL;
TupleDesc tupdesc;
tupdesc = RelationGetDescr(rel);
- if (tupdesc->constr && tupdesc->constr->has_generated_virtual)
+ if (tupdesc->constr &&
+ (tupdesc->constr->has_generated_virtual ||
+ (include_stored && tupdesc->constr->has_generated_stored)))
{
- List *tlist = NIL;
-
for (int i = 0; i < tupdesc->natts; i++)
{
Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
- if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL ||
+ (include_stored && attr->attgenerated == ATTRIBUTE_GENERATED_STORED))
{
Node *defexpr;
TargetEntry *te;
@@ -4568,19 +4628,12 @@ expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
ChangeVarNodes(defexpr, 1, rt_index, 0);
te = makeTargetEntry((Expr *) defexpr, i + 1, 0, false);
- tlist = lappend(tlist, te);
+ gen_cols = lappend(gen_cols, te);
}
}
-
- Assert(list_length(tlist) > 0);
-
- node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, tlist,
- result_relation,
- REPLACEVARS_CHANGE_VARNO, rt_index,
- NULL);
}
- return node;
+ return gen_cols;
}
/*
@@ -4597,6 +4650,7 @@ expand_generated_columns_in_expr(Node *node, Relation rel, int rt_index)
if (tupdesc->constr && tupdesc->constr->has_generated_virtual)
{
RangeTblEntry *rte;
+ List *vcols;
rte = makeNode(RangeTblEntry);
/* eref needs to be set, but the actual name doesn't matter */
@@ -4604,14 +4658,26 @@ expand_generated_columns_in_expr(Node *node, Relation rel, int rt_index)
rte->rtekind = RTE_RELATION;
rte->relid = RelationGetRelid(rel);
- node = expand_generated_columns_internal(node, rel, rt_index, rte, 0);
+ vcols = get_generated_columns(rel, rt_index, false);
+
+ if (vcols)
+ {
+ /*
+ * Passing NULL for outer_hasSubLinks is safe because generation
+ * expressions cannot contain SubLinks, so the replacement cannot
+ * introduce any.
+ */
+ node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, vcols, 0,
+ REPLACEVARS_CHANGE_VARNO, rt_index,
+ NULL);
+ }
}
return node;
}
/*
- * Build the generation expression for the virtual generated column.
+ * Build the generation expression for a generated column.
*
* Error out if there is no generation expression found for the given column.
*/
@@ -4623,8 +4689,11 @@ build_generation_expression(Relation rel, int attrno)
Node *defexpr;
Oid attcollid;
- Assert(rd_att->constr && rd_att->constr->has_generated_virtual);
- Assert(att_tup->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL);
+ Assert(rd_att->constr &&
+ (rd_att->constr->has_generated_virtual ||
+ rd_att->constr->has_generated_stored));
+ Assert(att_tup->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL ||
+ att_tup->attgenerated == ATTRIBUTE_GENERATED_STORED);
defexpr = build_column_default(rel, attrno);
if (defexpr == NULL)
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index 4d329c60994..7866ae0ebbe 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1604,6 +1604,40 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
c | integer | | |
x | integer | | | generated always as (b * 2) stored
+-- rule actions referring to generated columns:
+-- NEW.b in a rule action should reflect the generated column's new value
+CREATE TABLE gtest_rule (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
+CREATE TABLE gtest_rule_log (op text, old_b int, new_b int);
+CREATE RULE gtest_rule_upd AS ON UPDATE TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('UPD', OLD.b, NEW.b);
+CREATE RULE gtest_rule_ins AS ON INSERT TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('INS', NULL, NEW.b);
+INSERT INTO gtest_rule (a) VALUES (1);
+UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET a = (SELECT max(b) FROM gtest_rule);
+SELECT * FROM gtest_rule_log;
+ op | old_b | new_b
+-----+-------+-------
+ INS | | 2
+ UPD | 2 | 20
+ UPD | 20 | 40
+(3 rows)
+
+DROP RULE gtest_rule_upd ON gtest_rule;
+DROP RULE gtest_rule_ins ON gtest_rule;
+DROP TABLE gtest_rule_log;
+-- rule quals referring to generated columns:
+-- NEW.b in the rule qual should reflect the generated column's new value
+CREATE RULE gtest_rule_qual AS ON UPDATE TO gtest_rule WHERE NEW.b > 100
+ DO INSTEAD NOTHING;
+UPDATE gtest_rule SET a = 100;
+SELECT * FROM gtest_rule;
+ a | b
+----+----
+ 20 | 40
+(1 row)
+
+DROP TABLE gtest_rule;
-- sanity check of system catalog
SELECT attrelid, attname, attgenerated FROM pg_attribute WHERE attgenerated NOT IN ('', 's', 'v');
attrelid | attname | attgenerated
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index fc41c480d40..0d9c76c4e77 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1526,6 +1526,40 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
c | integer | | |
x | integer | | | generated always as (b * 2)
+-- rule actions referring to generated columns:
+-- NEW.b in a rule action should reflect the generated column's new value
+CREATE TABLE gtest_rule (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
+CREATE TABLE gtest_rule_log (op text, old_b int, new_b int);
+CREATE RULE gtest_rule_upd AS ON UPDATE TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('UPD', OLD.b, NEW.b);
+CREATE RULE gtest_rule_ins AS ON INSERT TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('INS', NULL, NEW.b);
+INSERT INTO gtest_rule (a) VALUES (1);
+UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET a = (SELECT max(b) FROM gtest_rule);
+SELECT * FROM gtest_rule_log;
+ op | old_b | new_b
+-----+-------+-------
+ INS | | 2
+ UPD | 2 | 20
+ UPD | 20 | 40
+(3 rows)
+
+DROP RULE gtest_rule_upd ON gtest_rule;
+DROP RULE gtest_rule_ins ON gtest_rule;
+DROP TABLE gtest_rule_log;
+-- rule quals referring to generated columns:
+-- NEW.b in the rule qual should reflect the generated column's new value
+CREATE RULE gtest_rule_qual AS ON UPDATE TO gtest_rule WHERE NEW.b > 100
+ DO INSTEAD NOTHING;
+UPDATE gtest_rule SET a = 100;
+SELECT * FROM gtest_rule;
+ a | b
+----+----
+ 20 | 40
+(1 row)
+
+DROP TABLE gtest_rule;
-- sanity check of system catalog
SELECT attrelid, attname, attgenerated FROM pg_attribute WHERE attgenerated NOT IN ('', 's', 'v');
attrelid | attname | attgenerated
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index 1064839dcd2..6746cd4632b 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -801,6 +801,29 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
\d gtest28*
+-- rule actions referring to generated columns:
+-- NEW.b in a rule action should reflect the generated column's new value
+CREATE TABLE gtest_rule (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
+CREATE TABLE gtest_rule_log (op text, old_b int, new_b int);
+CREATE RULE gtest_rule_upd AS ON UPDATE TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('UPD', OLD.b, NEW.b);
+CREATE RULE gtest_rule_ins AS ON INSERT TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('INS', NULL, NEW.b);
+INSERT INTO gtest_rule (a) VALUES (1);
+UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET a = (SELECT max(b) FROM gtest_rule);
+SELECT * FROM gtest_rule_log;
+DROP RULE gtest_rule_upd ON gtest_rule;
+DROP RULE gtest_rule_ins ON gtest_rule;
+DROP TABLE gtest_rule_log;
+
+-- rule quals referring to generated columns:
+-- NEW.b in the rule qual should reflect the generated column's new value
+CREATE RULE gtest_rule_qual AS ON UPDATE TO gtest_rule WHERE NEW.b > 100
+ DO INSTEAD NOTHING;
+UPDATE gtest_rule SET a = 100;
+SELECT * FROM gtest_rule;
+DROP TABLE gtest_rule;
-- sanity check of system catalog
SELECT attrelid, attname, attgenerated FROM pg_attribute WHERE attgenerated NOT IN ('', 's', 'v');
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index 9b32413e3a9..1bac51ea731 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -814,6 +814,29 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
\d gtest28*
+-- rule actions referring to generated columns:
+-- NEW.b in a rule action should reflect the generated column's new value
+CREATE TABLE gtest_rule (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
+CREATE TABLE gtest_rule_log (op text, old_b int, new_b int);
+CREATE RULE gtest_rule_upd AS ON UPDATE TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('UPD', OLD.b, NEW.b);
+CREATE RULE gtest_rule_ins AS ON INSERT TO gtest_rule
+ DO ALSO INSERT INTO gtest_rule_log VALUES ('INS', NULL, NEW.b);
+INSERT INTO gtest_rule (a) VALUES (1);
+UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET a = (SELECT max(b) FROM gtest_rule);
+SELECT * FROM gtest_rule_log;
+DROP RULE gtest_rule_upd ON gtest_rule;
+DROP RULE gtest_rule_ins ON gtest_rule;
+DROP TABLE gtest_rule_log;
+
+-- rule quals referring to generated columns:
+-- NEW.b in the rule qual should reflect the generated column's new value
+CREATE RULE gtest_rule_qual AS ON UPDATE TO gtest_rule WHERE NEW.b > 100
+ DO INSTEAD NOTHING;
+UPDATE gtest_rule SET a = 100;
+SELECT * FROM gtest_rule;
+DROP TABLE gtest_rule;
-- sanity check of system catalog
SELECT attrelid, attname, attgenerated FROM pg_attribute WHERE attgenerated NOT IN ('', 's', 'v');
--
2.39.5 (Apple Git-154)
view thread (16+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Bug: Rule actions see wrong values for generated columns (NEW.gen reads OLD value)
In-Reply-To: <CAMbWs49GX=-p_dD=2zQvHB4yfjwCttWSXg8Pz8xcZJJYAjcp1Q@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox