public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Dean Rasheed <[email protected]>
Cc: Richard Guo <[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: Tue, 14 Apr 2026 10:49:38 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAEZATCWYqyJJJcMNEj-LEvCSwog2H_HS61hTROZLKYuaiy5YTg@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>
> On Apr 13, 2026, at 19:03, Dean Rasheed <[email protected]> wrote:
>
> On Mon, 13 Apr 2026, 09:20 Richard Guo, <[email protected]> wrote:
> 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
>
> One thing about that approach is that it leads to 2 full rewrites of the rule action using ReplaceVarsFromTargetList(). I think that could be avoided by using including generated column expressions in the targetlist passed to ReplaceVarsFromTargetList() by rewriteRuleAction(). I haven't tried it, but I imagine it could reuse some code from expand_generated_columns_internal().
>
> Regards,
> Dean
I think Dean’s comment aligns with my implementation. My implementation just adds generated columns back to target list when needed, and pass the new target list to ReplaceVarsFromTargetList.
Please see the attached diff for my implementation:
* The diff is based on Richard’s v1 patch.
* As this diff is only to show my implementation rather than proposing a patch, so the diff is not well polished, and I didn’t touch the change to expand_generated_columns_internal of v1 patch.
* The v1 patch has a typo in generated_virtual.sql where “STORED” should be “VIRTUAL”, I fixed that as it impacts the tests.
* I updated the test cases by adding one more column, so that the generated column is built upon two columns.
I am really new to this area, so please don’t be surprised if I missed something or made something wrong. I am trying to learn by resolving problems.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] chao_impl.diff (8.0K, 2-chao_impl.diff)
download | inline diff:
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 0ba9931b2fd..e7cf63cb47a 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -332,6 +332,57 @@ acquireLocksOnSubLinks(Node *node, acquireLocksOnSubLinks_context *context)
return expression_tree_walker(node, acquireLocksOnSubLinks, context);
}
+static List *
+add_generated_columns_to_targetlist(List *targetList,
+ CmdType commandType,
+ Relation target_relation,
+ RangeTblEntry *target_rte,
+ int target_rt_index,
+ int result_relation,
+ int nomatch_varno)
+{
+ TupleDesc tupdesc = RelationGetDescr(target_relation);
+ List *result;
+
+ if (!(tupdesc->constr &&
+ (tupdesc->constr->has_generated_stored ||
+ tupdesc->constr->has_generated_virtual)))
+ return targetList;
+
+ result = list_copy(targetList);
+
+ for (int i = 0; i < tupdesc->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
+ Node *expr;
+ TargetEntry *te;
+
+ if (!attr->attgenerated)
+ continue;
+
+ expr = build_generation_expression(target_relation, i + 1);
+ ChangeVarNodes(expr, 1, target_rt_index, 0);
+ expr = ReplaceVarsFromTargetList(expr,
+ target_rt_index,
+ 0,
+ target_rte,
+ targetList,
+ result_relation,
+ (commandType == CMD_UPDATE) ?
+ REPLACEVARS_CHANGE_VARNO :
+ REPLACEVARS_SUBSTITUTE_NULL,
+ nomatch_varno,
+ NULL);
+
+ te = makeTargetEntry((Expr *) expr,
+ i + 1,
+ pstrdup(NameStr(attr->attname)),
+ false);
+ result = lappend(result, te);
+ }
+
+ return result;
+}
/*
* rewriteRuleAction -
@@ -645,24 +696,16 @@ rewriteRuleAction(Query *parsetree,
{
RangeTblEntry *new_rte = rt_fetch(new_varno, sub_action->rtable);
Relation new_rel;
+ List *new_targetlist;
- /*
- * 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);
+ new_targetlist = add_generated_columns_to_targetlist(parsetree->targetList,
+ event,
+ new_rel,
+ new_rte,
+ new_varno,
+ sub_action->resultRelation,
+ current_varno);
relation_close(new_rel, NoLock);
sub_action = (Query *)
@@ -670,7 +713,7 @@ rewriteRuleAction(Query *parsetree,
new_varno,
0,
new_rte,
- parsetree->targetList,
+ new_targetlist,
sub_action->resultRelation,
(event == CMD_UPDATE) ?
REPLACEVARS_CHANGE_VARNO :
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index 363bd7ccad5..be421b738bc 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1600,20 +1600,22 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
-- 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 (a int, c int, b int GENERATED ALWAYS AS (a + c) 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);
+INSERT INTO gtest_rule (a, c) VALUES (1, 5);
UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET c = 7;
SELECT * FROM gtest_rule_log;
op | old_b | new_b
-----+-------+-------
- INS | | 2
- UPD | 2 | 20
-(2 rows)
+ INS | | 6
+ UPD | 6 | 15
+ UPD | 15 | 17
+(3 rows)
DROP TABLE gtest_rule, gtest_rule_log;
-- sanity check of system catalog
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index 5bbd2f684ce..35b54f98ca1 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1522,20 +1522,22 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
-- 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 (a int, c int, b int GENERATED ALWAYS AS (a + c) 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);
+INSERT INTO gtest_rule (a, c) VALUES (1, 5);
UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET c = 7;
SELECT * FROM gtest_rule_log;
op | old_b | new_b
-----+-------+-------
- INS | | 2
- UPD | 2 | 20
-(2 rows)
+ INS | | 6
+ UPD | 6 | 15
+ UPD | 15 | 17
+(3 rows)
DROP TABLE gtest_rule, gtest_rule_log;
-- sanity check of system catalog
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index 3265a1e88c6..bda1fe760e8 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -800,14 +800,15 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
-- 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 (a int, c int, b int GENERATED ALWAYS AS (a + c) 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);
+INSERT INTO gtest_rule (a, c) VALUES (1, 5);
UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET c = 7;
SELECT * FROM gtest_rule_log;
DROP TABLE gtest_rule, gtest_rule_log;
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index cd1cd938975..b7c4c711e61 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -813,14 +813,15 @@ CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);
-- 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 (a int, c int, b int GENERATED ALWAYS AS (a + c) 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);
+INSERT INTO gtest_rule (a, c) VALUES (1, 5);
UPDATE gtest_rule SET a = 10;
+UPDATE gtest_rule SET c = 7;
SELECT * FROM gtest_rule_log;
DROP TABLE gtest_rule, gtest_rule_log;
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: <[email protected]>
* 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