public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: 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, 13 Apr 2026 17:20:35 +0900
Message-ID: <CAMbWs48=QrLy8gZprTfCmEtzoaUku54JXvAcnoRSo+OC8vUpwA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAHg+QDexGTmCZzx=73gXkY2ZADS6LRhpnU+-8Y_QmrdTS6yUhA@mail.gmail.com>
<CAMbWs4-SJ6B26ciFu_K_2M1ObDA=GirV87N1BKeAtSRmQGATUA@mail.gmail.com>
<[email protected]>
On Mon, Apr 13, 2026 at 4:21 PM Chao Li <[email protected]> wrote:
> I think the issue is that rewriteTargetListIU() removes generated columns from the target list, as described by this comment:
> Later, when the rule action is rewritten, ReplaceVarsFromTargetList() cannot find a target list entry for NEW.gen. For UPDATE rules, the missing NEW column is handled with REPLACEVARS_CHANGE_VARNO, so it falls back to referencing the original target relation row, which gives the old value.
I came to the same conclusion.
> One possible fix is to build a new target list that adds generated columns back when there are rules to fire. I tried the solution locally with some quick and dirty code and it seems to fix both stored and virtual generated columns for me.
I think a simpler fix might be to expand generated column references
in the NEW relation to their generation expressions before
ReplaceVarsFromTargetList resolves NEW references, so that the base
column Vars within the expressions can be correctly resolved.
Something like attached.
- Richard
Attachments:
[application/octet-stream] v1-0001-Fix-incorrect-NEW-references-to-generated-columns.patch (11.0K, 2-v1-0001-Fix-incorrect-NEW-references-to-generated-columns.patch)
download | inline diff:
From 7cc29faf775b2368d4381856d6f853df01ab01c2 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Mon, 13 Apr 2026 16:54:23 +0900
Subject: [PATCH v1] Fix incorrect NEW references to generated columns in rule
actions
---
src/backend/rewrite/rewriteHandler.c | 75 ++++++++++++++-----
.../regress/expected/generated_stored.out | 18 +++++
.../regress/expected/generated_virtual.out | 18 +++++
src/test/regress/sql/generated_stored.sql | 13 ++++
src/test/regress/sql/generated_virtual.sql | 13 ++++
5 files changed, 118 insertions(+), 19 deletions(-)
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 021c73f1b67..0ba9931b2fd 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -98,7 +98,8 @@ static List *matchLocks(CmdType event, Relation relation,
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);
+ RangeTblEntry *rte, int result_relation,
+ bool include_stored);
/*
@@ -642,11 +643,33 @@ 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;
+
+ /*
+ * First, expand any generated column references in the NEW relation.
+ * This must happen before ReplaceVarsFromTargetList, because the
+ * query's target list won't contain entries for generated columns
+ * (they are removed by rewriteTargetListIU). Without this step,
+ * ReplaceVarsFromTargetList would fall through to
+ * REPLACEVARS_CHANGE_VARNO (for UPDATE) or
+ * REPLACEVARS_SUBSTITUTE_NULL (for INSERT), producing wrong results
+ * when the generated column depends on columns being modified.
+ */
+ new_rel = relation_open(new_rte->relid, NoLock);
+ sub_action = (Query *)
+ expand_generated_columns_internal((Node *) sub_action,
+ new_rel, new_varno,
+ new_rte,
+ sub_action->resultRelation,
+ true);
+ relation_close(new_rel, NoLock);
+
sub_action = (Query *)
ReplaceVarsFromTargetList((Node *) sub_action,
new_varno,
0,
- rt_fetch(new_varno, sub_action->rtable),
+ new_rte,
parsetree->targetList,
sub_action->resultRelation,
(event == CMD_UPDATE) ?
@@ -4530,28 +4553,38 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
/*
- * Expand virtual generated columns
+ * Expand generated columns
*
- * If the table contains virtual generated columns, build a target list
- * containing the expanded expressions and use ReplaceVarsFromTargetList() to
- * do the replacements.
+ * If the table contains 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.
+ * generated column expressions from rel, if there are any.
*
* 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.
+ *
+ * If include_stored is false, only virtual generated columns are expanded.
+ * If include_stored is true, both stored and virtual generated columns are
+ * expanded. The latter is needed when expanding NEW references in rule
+ * actions, because rewriteTargetListIU removes both types from the target
+ * list, and both need to be expanded before ReplaceVarsFromTargetList can
+ * correctly resolve them.
*/
static Node *
expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
- RangeTblEntry *rte, int result_relation)
+ RangeTblEntry *rte, int result_relation,
+ bool include_stored)
{
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;
@@ -4559,7 +4592,8 @@ expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
{
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;
@@ -4572,12 +4606,11 @@ expand_generated_columns_internal(Node *node, Relation rel, int rt_index,
}
}
- Assert(list_length(tlist) > 0);
-
- node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, tlist,
- result_relation,
- REPLACEVARS_CHANGE_VARNO, rt_index,
- NULL);
+ if (tlist != NIL)
+ node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, tlist,
+ result_relation,
+ REPLACEVARS_CHANGE_VARNO, rt_index,
+ NULL);
}
return node;
@@ -4604,7 +4637,8 @@ 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);
+ node = expand_generated_columns_internal(node, rel, rt_index, rte, 0,
+ false);
}
return node;
@@ -4623,8 +4657,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 43cddeac373..363bd7ccad5 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1598,6 +1598,24 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
c | integer | | |
x | integer | | | generated always as (b * 2) stored
+-- rules
+-- 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;
+SELECT * FROM gtest_rule_log;
+ op | old_b | new_b
+-----+-------+-------
+ INS | | 2
+ UPD | 2 | 20
+(2 rows)
+
+DROP TABLE gtest_rule, gtest_rule_log;
-- 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 234061fa1f7..5bbd2f684ce 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1520,6 +1520,24 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
c | integer | | |
x | integer | | | generated always as (b * 2)
+-- rules
+-- 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;
+SELECT * FROM gtest_rule_log;
+ op | old_b | new_b
+-----+-------+-------
+ INS | | 2
+ UPD | 2 | 20
+(2 rows)
+
+DROP TABLE gtest_rule, gtest_rule_log;
-- 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 280021d79b7..3265a1e88c6 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -798,6 +798,19 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
\d gtest28*
+-- rules
+-- 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;
+SELECT * FROM gtest_rule_log;
+DROP TABLE gtest_rule, gtest_rule_log;
+
-- 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 4d9ad3c5dca..cd1cd938975 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -811,6 +811,19 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
\d gtest28*
+-- rules
+-- 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;
+SELECT * FROM gtest_rule_log;
+DROP TABLE gtest_rule, gtest_rule_log;
+
-- 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]
Subject: Re: Bug: Rule actions see wrong values for generated columns (NEW.gen reads OLD value)
In-Reply-To: <CAMbWs48=QrLy8gZprTfCmEtzoaUku54JXvAcnoRSo+OC8vUpwA@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