On Tue, 14 Apr 2026 at 07:29, Chao Li <[email protected]> wrote: > > > On Apr 14, 2026, at 11:27, Richard Guo <[email protected]> wrote: > > > > It could be made to work by pre-resolving the generation expressions' > > base column Vars before adding them to the UPDATE's targetlist. For > > each generated column, we'd call ReplaceVarsFromTargetList on the > > generation expression to resolve its base column Vars, then add the > > fully resolved expression to the targetlist. But this seems to add > > code complexity. And I'm not sure about the performance difference > > between these two approaches. I expect that rule action trees are > > typically small. > > > My implementation has pre-resolved the generation expressions, that’s why all > tests passed. But I agree my change is heavier as I had to add a new static > helper function. > > If we think rule actions are usually small enough that the extra full-tree > pass would not be an issue, then v1 may be preferable for simplicity. > > My only comment on v1 is the typo in generated_virtual.sql where “STORED” > should be “VIRTUAL”. >
I don't quite buy the argument that the rule action is typically small. We have no idea how big it might be. Note that, at that point in the code, sub_action is the combination of both the original query and the rule action. I do accept though that rules are not widely used, and that it's not worth optimising too much, if it means a lot of extra complexity. However, IMO, it is slightly simpler and neater to put the expanded generated columns in the replacement list used by ReplaceVarsFromTargetList() on sub_action. In the attached v2 patch, I've done that by refactoring expand_generated_columns_internal(), renaming it to get_generated_columns(), and making it just return the list of generated column expressions, rather than doing the rewrite -- I never particularly liked the separation of concerns between expand_generated_columns_internal() and expand_generated_columns_in_expr(), especially after the rest of the code expanding virtual generated columns was moved out of the rewriter, so that expand_generated_columns_in_expr() became the only caller of expand_generated_columns_internal(). Doing this simplifies the function, since it's no longer necessary to pass it node, rte, and result_relation. With that change, all rewriteRuleAction() needs to do is get the generated columns, rewrite any new.attribute references in them, and then use that list plus the original target list as the replacement list when rewriting sub_action. It is a slightly bigger patch overall, but it feels a little neater and more logical to me, but I accept that that's a subjective thing. Note: I included a minor comment update needed for build_generation_expression(), and the test fix noted above. Regards, Dean
From 2e36ee327a7971610ac395418c9e22bf8145850d Mon Sep 17 00:00:00 2001 From: Dean Rasheed <[email protected]> Date: Fri, 17 Apr 2026 10:30:14 +0100 Subject: [PATCH v2] Fix incorrect NEW references to generated columns in rule actions. --- src/backend/rewrite/rewriteHandler.c | 100 ++++++++++++------ .../regress/expected/generated_stored.out | 18 ++++ .../regress/expected/generated_virtual.out | 18 ++++ src/test/regress/sql/generated_stored.sql | 12 +++ src/test/regress/sql/generated_virtual.sql | 12 +++ 5 files changed, 125 insertions(+), 35 deletions(-) diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 021c73f1b67..cd950fd4a07 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 typically 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, + NULL); + + /* + * 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 : @@ -4530,36 +4563,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 +4596,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 +4618,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 +4626,19 @@ 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) + 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 +4650,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..43535561a98 100644 --- a/src/test/regress/expected/generated_stored.out +++ b/src/test/regress/expected/generated_stored.out @@ -1604,6 +1604,24 @@ 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; +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 fc41c480d40..cd482d6ee03 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -1526,6 +1526,24 @@ 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; +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 1064839dcd2..795c7e4cd66 100644 --- a/src/test/regress/sql/generated_stored.sql +++ b/src/test/regress/sql/generated_stored.sql @@ -801,6 +801,18 @@ 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; +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 9b32413e3a9..37b5af2497d 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -814,6 +814,18 @@ 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; +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.43.0
