Re: BUG #16671: "generated always as" is ignored when updating table through view

I wrote:
>> PG Bug reporting form <noreply@postgresql.org> writes:
>>> [ updating via a view fails to recalculate GENERATED columns ]

>> Yeah, that's surely a bug.  In fact, it's a regression, because
>> the test case works as-expected in v12.  Not sure where we broke it.

> git bisect blames
> c6679e4fca21d253ced84c51ac1a31c1b2aec72f is the first bad commit

On digging into this, I see that it's one of the issues I complained
about in [1].  Here is a patch that proposes to fix that by moving
calculation of extraUpdatedCols into the query rewriter.

(With this patch, stored extraUpdatedCols values would always be
null.  But it wouldn't matter if we are working with a rule in which
the field isn't null, since the rewriter would just overwrite it.)

            regards, tom lane

[1] https://www.postgresql.org/message-id/2351.1589824639%40sss.pgh.pa.us

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 8b43371425..127ea3d856 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -438,9 +438,9 @@ flatten_rtes_walker(Node *node, PlannerGlobal *glob)
  * In the flat rangetable, we zero out substructure pointers that are not
  * needed by the executor; this reduces the storage space and copying cost
  * for cached plans.  We keep only the ctename, alias and eref Alias fields,
- * which are needed by EXPLAIN, and the selectedCols, insertedCols and
- * updatedCols bitmaps, which are needed for executor-startup permissions
- * checking and for trigger event checking.
+ * which are needed by EXPLAIN, and the selectedCols, insertedCols,
+ * updatedCols, and extraUpdatedCols bitmaps, which are needed for
+ * executor-startup permissions checking and for trigger event checking.
  */
 static void
 add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 98a83db8b5..575e22ce0d 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -2282,7 +2282,6 @@ transformUpdateTargetList(ParseState *pstate, List *origTlist)
     RangeTblEntry *target_rte;
     ListCell   *orig_tl;
     ListCell   *tl;
-    TupleDesc    tupdesc = pstate->p_target_relation->rd_att;

     tlist = transformTargetList(pstate, origTlist,
                                 EXPR_KIND_UPDATE_SOURCE);
@@ -2341,41 +2340,9 @@ transformUpdateTargetList(ParseState *pstate, List *origTlist)
     if (orig_tl != NULL)
         elog(ERROR, "UPDATE target count mismatch --- internal error");

-    fill_extraUpdatedCols(target_rte, tupdesc);
-
     return tlist;
 }

-/*
- * Record in extraUpdatedCols generated columns referencing updated base
- * columns.
- */
-void
-fill_extraUpdatedCols(RangeTblEntry *target_rte, TupleDesc tupdesc)
-{
-    if (tupdesc->constr &&
-        tupdesc->constr->has_generated_stored)
-    {
-        for (int i = 0; i < tupdesc->constr->num_defval; i++)
-        {
-            AttrDefault defval = tupdesc->constr->defval[i];
-            Node       *expr;
-            Bitmapset  *attrs_used = NULL;
-
-            /* skip if not generated column */
-            if (!TupleDescAttr(tupdesc, defval.adnum - 1)->attgenerated)
-                continue;
-
-            expr = stringToNode(defval.adbin);
-            pull_varattnos(expr, 1, &attrs_used);
-
-            if (bms_overlap(target_rte->updatedCols, attrs_used))
-                target_rte->extraUpdatedCols = bms_add_member(target_rte->extraUpdatedCols,
-                                                              defval.adnum - FirstLowInvalidHeapAttributeNumber);
-        }
-    }
-}
-
 /*
  * transformReturningList -
  *    handle a RETURNING clause in INSERT/UPDATE/DELETE
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 3a5b733ee3..b0f27e0af8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -81,8 +81,6 @@
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "optimizer/optimizer.h"
-#include "parser/analyze.h"
-#include "parser/parse_relation.h"
 #include "pgstat.h"
 #include "postmaster/bgworker.h"
 #include "postmaster/interrupt.h"
@@ -1323,7 +1321,8 @@ apply_handle_update(StringInfo s)
         }
     }

-    fill_extraUpdatedCols(target_rte, RelationGetDescr(rel->localrel));
+    /* Also populate extraUpdatedCols, in case we have generated columns */
+    fill_extraUpdatedCols(target_rte, rel->localrel);

     PushActiveSnapshot(GetTransactionSnapshot());

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 1faaafab08..41dd670572 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -30,6 +30,7 @@
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
+#include "optimizer/optimizer.h"
 #include "parser/analyze.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_relation.h"
@@ -1508,6 +1509,42 @@ rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
     }
 }

+/*
+ * Record in target_rte->extraUpdatedCols the indexes of any generated columns
+ * that depend on any columns mentioned in target_rte->updatedCols.
+ */
+void
+fill_extraUpdatedCols(RangeTblEntry *target_rte, Relation target_relation)
+{
+    TupleDesc    tupdesc = RelationGetDescr(target_relation);
+    TupleConstr *constr = tupdesc->constr;
+
+    target_rte->extraUpdatedCols = NULL;
+
+    if (constr && constr->has_generated_stored)
+    {
+        for (int i = 0; i < constr->num_defval; i++)
+        {
+            AttrDefault *defval = &constr->defval[i];
+            Node       *expr;
+            Bitmapset  *attrs_used = NULL;
+
+            /* skip if not generated column */
+            if (!TupleDescAttr(tupdesc, defval->adnum - 1)->attgenerated)
+                continue;
+
+            /* identify columns this generated column depends on */
+            expr = stringToNode(defval->adbin);
+            pull_varattnos(expr, 1, &attrs_used);
+
+            if (bms_overlap(target_rte->updatedCols, attrs_used))
+                target_rte->extraUpdatedCols =
+                    bms_add_member(target_rte->extraUpdatedCols,
+                                   defval->adnum - FirstLowInvalidHeapAttributeNumber);
+        }
+    }
+}
+

 /*
  * matchLocks -
@@ -1639,6 +1676,7 @@ ApplyRetrieveRule(Query *parsetree,
             rte->selectedCols = NULL;
             rte->insertedCols = NULL;
             rte->updatedCols = NULL;
+            rte->extraUpdatedCols = NULL;

             /*
              * For the most part, Vars referencing the view should remain as
@@ -3617,6 +3655,9 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
                                     parsetree->override,
                                     rt_entry_relation,
                                     parsetree->resultRelation);
+
+            /* Also populate extraUpdatedCols (for generated columns) */
+            fill_extraUpdatedCols(rt_entry, rt_entry_relation);
         }
         else if (event == CMD_DELETE)
         {
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 60c2f45466..ff584f2955 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -940,12 +940,16 @@ typedef struct PartitionCmd
  *
  *      updatedCols is also used in some other places, for example, to determine
  *      which triggers to fire and in FDWs to know which changed columns they
- *      need to ship off.  Generated columns that are caused to be updated by an
- *      update to a base column are collected in extraUpdatedCols.  This is not
- *      considered for permission checking, but it is useful in those places
- *      that want to know the full set of columns being updated as opposed to
- *      only the ones the user explicitly mentioned in the query.  (There is
- *      currently no need for an extraInsertedCols, but it could exist.)
+ *      need to ship off.
+ *
+ *      Generated columns that are caused to be updated by an update to a base
+ *      column are listed in extraUpdatedCols.  This is not considered for
+ *      permission checking, but it is useful in those places that want to know
+ *      the full set of columns being updated as opposed to only the ones the
+ *      user explicitly mentioned in the query.  (There is currently no need for
+ *      an extraInsertedCols, but it could exist.)  Note that extraUpdatedCols
+ *      is populated during query rewrite, NOT in the parser, since generated
+ *      columns could be added after a rule has been parsed and stored.
  *
  *      securityQuals is a list of security barrier quals (boolean expressions),
  *      to be tested in the listed order before returning a row from the
diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h
index 9d09a02141..d6a467a572 100644
--- a/src/include/parser/analyze.h
+++ b/src/include/parser/analyze.h
@@ -46,6 +46,4 @@ extern void applyLockingClause(Query *qry, Index rtindex,
 extern List *BuildOnConflictExcludedTargetlist(Relation targetrel,
                                                Index exclRelIndex);

-extern void fill_extraUpdatedCols(RangeTblEntry *target_rte, TupleDesc tupdesc);
-
 #endif                            /* ANALYZE_H */
diff --git a/src/include/rewrite/rewriteHandler.h b/src/include/rewrite/rewriteHandler.h
index eb2e7b1768..a18211f4a2 100644
--- a/src/include/rewrite/rewriteHandler.h
+++ b/src/include/rewrite/rewriteHandler.h
@@ -26,6 +26,9 @@ extern Node *build_column_default(Relation rel, int attrno);
 extern void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
                                 Relation target_relation);

+extern void fill_extraUpdatedCols(RangeTblEntry *target_rte,
+                                  Relation target_relation);
+
 extern Query *get_view_query(Relation view);
 extern const char *view_query_is_auto_updatable(Query *viewquery,
                                                 bool check_cols);
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index caed1c19ec..6a977006ef 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1467,6 +1467,41 @@ NOTICE:  drop cascades to 3 other objects
 DETAIL:  drop cascades to view rw_view1
 drop cascades to view rw_view2
 drop cascades to view rw_view3
+-- view on table with GENERATED columns
+CREATE TABLE base_tbl (id int, idplus1 int GENERATED ALWAYS AS (id + 1) STORED);
+CREATE VIEW rw_view1 AS SELECT * FROM base_tbl;
+INSERT INTO base_tbl (id) VALUES (1);
+INSERT INTO rw_view1 (id) VALUES (2);
+INSERT INTO base_tbl (id, idplus1) VALUES (3, DEFAULT);
+INSERT INTO rw_view1 (id, idplus1) VALUES (4, DEFAULT);
+INSERT INTO base_tbl (id, idplus1) VALUES (5, 6);  -- error
+ERROR:  cannot insert into column "idplus1"
+DETAIL:  Column "idplus1" is a generated column.
+INSERT INTO rw_view1 (id, idplus1) VALUES (6, 7);  -- error
+ERROR:  cannot insert into column "idplus1"
+DETAIL:  Column "idplus1" is a generated column.
+SELECT * FROM base_tbl;
+ id | idplus1
+----+---------
+  1 |       2
+  2 |       3
+  3 |       4
+  4 |       5
+(4 rows)
+
+UPDATE base_tbl SET id = 2000 WHERE id = 2;
+UPDATE rw_view1 SET id = 3000 WHERE id = 3;
+SELECT * FROM base_tbl;
+  id  | idplus1
+------+---------
+    1 |       2
+    4 |       5
+ 2000 |    2001
+ 3000 |    3001
+(4 rows)
+
+DROP TABLE base_tbl CASCADE;
+NOTICE:  drop cascades to view rw_view1
 -- inheritance tests
 CREATE TABLE base_tbl_parent (a int);
 CREATE TABLE base_tbl_child (CHECK (a > 0)) INHERITS (base_tbl_parent);
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index 64f23d0902..09328e582b 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -697,6 +697,27 @@ SELECT events & 4 != 0 AS upd,

 DROP TABLE base_tbl CASCADE;

+-- view on table with GENERATED columns
+
+CREATE TABLE base_tbl (id int, idplus1 int GENERATED ALWAYS AS (id + 1) STORED);
+CREATE VIEW rw_view1 AS SELECT * FROM base_tbl;
+
+INSERT INTO base_tbl (id) VALUES (1);
+INSERT INTO rw_view1 (id) VALUES (2);
+INSERT INTO base_tbl (id, idplus1) VALUES (3, DEFAULT);
+INSERT INTO rw_view1 (id, idplus1) VALUES (4, DEFAULT);
+INSERT INTO base_tbl (id, idplus1) VALUES (5, 6);  -- error
+INSERT INTO rw_view1 (id, idplus1) VALUES (6, 7);  -- error
+
+SELECT * FROM base_tbl;
+
+UPDATE base_tbl SET id = 2000 WHERE id = 2;
+UPDATE rw_view1 SET id = 3000 WHERE id = 3;
+
+SELECT * FROM base_tbl;
+
+DROP TABLE base_tbl CASCADE;
+
 -- inheritance tests

 CREATE TABLE base_tbl_parent (a int);