Calculate extraUpdatedCols in query rewriter, not parser.
authorTom Lane
Wed, 28 Oct 2020 17:47:02 +0000 (13:47 -0400)
committerTom Lane
Wed, 28 Oct 2020 17:47:02 +0000 (13:47 -0400)
It's unsafe to do this at parse time because addition of generated
columns to a table would not invalidate stored rules containing
UPDATEs on the table ... but there might now be dependent generated
columns that were not there when the rule was made.  This also fixes
an oversight that rewriteTargetView failed to update extraUpdatedCols
when transforming an UPDATE on an updatable view.  (Since the new
calculation is downstream of that, rewriteTargetView doesn't actually
need to do anything; but before, there was a demonstrable bug there.)

In v13 and HEAD, this leads to easily-visible bugs because (since
commit c6679e4fc) we won't recalculate generated columns that aren't
listed in extraUpdatedCols.  In v12 this bitmap is mostly just used
for trigger-firing decisions, so you'd only notice a problem if a
trigger cared whether a generated column had been updated.

I'd complained about this back in May, but then forgot about it
until bug #16671 from Michael Paul Killian revived the issue.

Back-patch to v12 where this field was introduced.  If existing
stored rules contain any extraUpdatedCols values, they'll be
ignored because the rewriter will overwrite them, so the bug will
be fixed even for existing rules.  (But note that if someone were
to update to 13.1 or 12.5, store some rules with UPDATEs on tables
having generated columns, and then downgrade to a prior minor version,
they might observe issues similar to what this patch fixes.  That
seems unlikely enough to not be worth going to a lot of effort to fix.)

Discussion: https://postgr.es/m/10206.1588964727@sss.pgh.pa.us
Discussion: https://postgr.es/m/16671-2fa55851859fb166@postgresql.org

src/backend/optimizer/plan/setrefs.c
src/backend/parser/analyze.c
src/backend/replication/logical/worker.c
src/backend/rewrite/rewriteHandler.c
src/include/nodes/parsenodes.h
src/include/parser/analyze.h
src/include/rewrite/rewriteHandler.h
src/test/regress/expected/updatable_views.out
src/test/regress/sql/updatable_views.sql

index 329ebd5f287e9fec12236b4120abbffec358514e..95a1c43c969fba69a2cd5991211be89ed0044994 100644 (file)
@@ -387,9 +387,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)
index ecd6a8bae7da35d7514d54a6dd0ae5a41b1d654e..8462e9155843c8a937aef36449b9593e974f2e7a 100644 (file)
@@ -2287,7 +2287,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);
@@ -2346,41 +2345,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
index 1bdf763d5854797a43f6af0dd2aeed8457c18b46..ce8f4106aa197776df6e41dac0981601b4ff8c5d 100644 (file)
@@ -42,8 +42,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/postmaster.h"
@@ -744,7 +742,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());
    ExecOpenIndices(estate->es_result_relation_info, false);
index bc49f23cc8f64bc7fd4a9df16687b1274d2461c3..987501136974c17acfa76976225f9b391c04da28 100644 (file)
@@ -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"
@@ -1510,6 +1511,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 -
@@ -1641,6 +1678,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)
        {
index 788e276227c92d9124f578447d63bf7ffbb35ff8..439dfea29adf27ecc6031cdc75ff2f2aacaf3107 100644 (file)
@@ -937,12 +937,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
index b3bad9716406666e9ee52e5f4298ddce90b6dda8..cb1d96bc35d9d80c3f53eb98fe61fc05618ef90a 100644 (file)
@@ -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 */
index 77b2ffa962156c7fdec1c9d1363d73144d01696d..ef01740fbd350368919b8b59ecb0e383751d6c8c 100644 (file)
@@ -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);
index a8833eb4fb6985e28b93655297e0f8635c5fdd77..0d16e68084af360d2679baece362b1bec5d6c7e7 100644 (file)
@@ -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);
index 64f23d0902ec946c91676d808ecd5707ce5699ab..09328e582b27b2bde07d24e44cfa129654144ea2 100644 (file)
@@ -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);