Yet further fixes for multi-row VALUES lists for updatable views.
authorTom Lane <[email protected]>
Tue, 11 Oct 2022 22:24:14 +0000 (18:24 -0400)
committerTom Lane <[email protected]>
Tue, 11 Oct 2022 22:24:14 +0000 (18:24 -0400)
DEFAULT markers appearing in an INSERT on an updatable view
could be mis-processed if they were in a multi-row VALUES clause.
This would lead to strange errors such as "cache lookup failed
for type NNNN", or in older branches even to crashes.

The cause is that commit 41531e42d tried to re-use rewriteValuesRTE()
to remove any SetToDefault nodes (that hadn't previously been replaced
by the view's own default values) appearing in "product" queries,
that is DO ALSO queries.  That's fundamentally wrong because the
DO ALSO queries might not even be INSERTs; and even if they are,
their targetlists don't necessarily match the view's column list,
so that almost all the logic in rewriteValuesRTE() is inapplicable.

What we want is a narrow focus on replacing any such nodes with NULL
constants.  (That is, in this context we are interpreting the defaults
as being strictly those of the view itself; and we already replaced
any that aren't NULL.)  We could add still more !force_nulls tests
to further lobotomize rewriteValuesRTE(); but it seems cleaner to
split out this case to a new function, restoring rewriteValuesRTE()
to the charter it had before.

Per bug #17633 from jiye_sw.  Patch by me, but thanks to
Richard Guo and Japin Li for initial investigation.
Back-patch to all supported branches, as the previous fix was.

Discussion: https://postgr.es/m/17633-98cc85e1fa91e905@postgresql.org

src/backend/rewrite/rewriteHandler.c
src/test/regress/expected/updatable_views.out
src/test/regress/sql/updatable_views.sql

index d02fd83c0ae5185836ec3f76930caf92c3d157a2..fb0c687bd854f0e9eb7cc707d2b2b18158a16c91 100644 (file)
@@ -79,8 +79,9 @@ static TargetEntry *process_matched_tle(TargetEntry *src_tle,
 static Node *get_assignment_input(Node *node);
 static Bitmapset *findDefaultOnlyColumns(RangeTblEntry *rte);
 static bool rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
-                            Relation target_relation, bool force_nulls,
+                            Relation target_relation,
                             Bitmapset *unused_cols);
+static void rewriteValuesRTEToNulls(Query *parsetree, RangeTblEntry *rte);
 static void markQueryForLocking(Query *qry, Node *jtnode,
                                LockClauseStrength strength, LockWaitPolicy waitPolicy,
                                bool pushedDown);
@@ -1370,18 +1371,7 @@ findDefaultOnlyColumns(RangeTblEntry *rte)
  * all DEFAULT items are replaced, and if the target relation doesn't have a
  * default, the value is explicitly set to NULL.
  *
- * Additionally, if force_nulls is true, the target relation's defaults are
- * ignored and all DEFAULT items in the VALUES list are explicitly set to
- * NULL, regardless of the target relation's type.  This is used for the
- * product queries generated by DO ALSO rules attached to an auto-updatable
- * view, for which we will have already called this function with force_nulls
- * false.  For these product queries, we must then force any remaining DEFAULT
- * items to NULL to provide concrete values for the rule actions.
- * Essentially, this is a mix of the 2 cases above --- the original query is
- * an insert into an auto-updatable view, and the product queries are inserts
- * into a rule-updatable view.
- *
- * Finally, if a DEFAULT item is found in a column mentioned in unused_cols,
+ * Also, if a DEFAULT item is found in a column mentioned in unused_cols,
  * it is explicitly set to NULL.  This happens for columns in the VALUES RTE
  * whose corresponding targetlist entries have already been replaced with the
  * relation's default expressions, so that any values in those columns of the
@@ -1404,7 +1394,7 @@ findDefaultOnlyColumns(RangeTblEntry *rte)
  */
 static bool
 rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
-                Relation target_relation, bool force_nulls,
+                Relation target_relation,
                 Bitmapset *unused_cols)
 {
    List       *newValues;
@@ -1414,15 +1404,16 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
    int         numattrs;
    int        *attrnos;
 
+   /* Steps below are not sensible for non-INSERT queries */
+   Assert(parsetree->commandType == CMD_INSERT);
+   Assert(rte->rtekind == RTE_VALUES);
+
    /*
     * Rebuilding all the lists is a pretty expensive proposition in a big
     * VALUES list, and it's a waste of time if there aren't any DEFAULT
     * placeholders.  So first scan to see if there are any.
-    *
-    * We skip this check if force_nulls is true, because we know that there
-    * are DEFAULT items present in that case.
     */
-   if (!force_nulls && !searchForDefault(rte))
+   if (!searchForDefault(rte))
        return true;            /* nothing to do */
 
    /*
@@ -1456,12 +1447,10 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
    /*
     * Check if the target relation is an auto-updatable view, in which case
     * unresolved defaults will be left untouched rather than being set to
-    * NULL.  If force_nulls is true, we always set DEFAULT items to NULL, so
-    * skip this check in that case --- it isn't an auto-updatable view.
+    * NULL.
     */
    isAutoUpdatableView = false;
-   if (!force_nulls &&
-       target_relation->rd_rel->relkind == RELKIND_VIEW &&
+   if (target_relation->rd_rel->relkind == RELKIND_VIEW &&
        !view_has_instead_trigger(target_relation, CMD_INSERT))
    {
        List       *locks;
@@ -1535,9 +1524,10 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
 
                if (attrno == 0)
                    elog(ERROR, "cannot set value in column %d to DEFAULT", i);
+               Assert(attrno > 0 && attrno <= target_relation->rd_att->natts);
                att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);
 
-               if (!force_nulls && !att_tup->attisdropped)
+               if (!att_tup->attisdropped)
                    new_expr = build_column_default(target_relation, attrno);
                else
                    new_expr = NULL;    /* force a NULL if dropped */
@@ -1587,6 +1577,50 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
    return allReplaced;
 }
 
+/*
+ * Mop up any remaining DEFAULT items in the given VALUES RTE by
+ * replacing them with NULL constants.
+ *
+ * This is used for the product queries generated by DO ALSO rules attached to
+ * an auto-updatable view.  The action can't depend on the "target relation"
+ * since the product query might not have one (it needn't be an INSERT).
+ * Essentially, such queries are treated as being attached to a rule-updatable
+ * view.
+ */
+static void
+rewriteValuesRTEToNulls(Query *parsetree, RangeTblEntry *rte)
+{
+   List       *newValues;
+   ListCell   *lc;
+
+   Assert(rte->rtekind == RTE_VALUES);
+   newValues = NIL;
+   foreach(lc, rte->values_lists)
+   {
+       List       *sublist = (List *) lfirst(lc);
+       List       *newList = NIL;
+       ListCell   *lc2;
+
+       foreach(lc2, sublist)
+       {
+           Node       *col = (Node *) lfirst(lc2);
+
+           if (IsA(col, SetToDefault))
+           {
+               SetToDefault *def = (SetToDefault *) col;
+
+               newList = lappend(newList, makeNullConst(def->typeId,
+                                                        def->typeMod,
+                                                        def->collation));
+           }
+           else
+               newList = lappend(newList, col);
+       }
+       newValues = lappend(newValues, newList);
+   }
+   rte->values_lists = newValues;
+}
+
 
 /*
  * Record in target_rte->extraUpdatedCols the indexes of any generated columns
@@ -3746,7 +3780,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
                                                            &unused_values_attrnos);
                /* ... and the VALUES expression lists */
                if (!rewriteValuesRTE(parsetree, values_rte, values_rte_index,
-                                     rt_entry_relation, false,
+                                     rt_entry_relation,
                                      unused_values_attrnos))
                    defaults_remaining = true;
            }
@@ -3860,10 +3894,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
                RangeTblEntry *values_rte = rt_fetch(values_rte_index,
                                                     pt->rtable);
 
-               rewriteValuesRTE(pt, values_rte, values_rte_index,
-                                rt_entry_relation,
-                                true,  /* Force remaining defaults to NULL */
-                                NULL);
+               rewriteValuesRTEToNulls(pt, values_rte);
            }
        }
 
index d57eeb761c074e35972b5af395e8da6ae56f8110..5a47dacad92585b34493f5eb4acb8230c381b084 100644 (file)
@@ -433,8 +433,30 @@ EXPLAIN (costs off) DELETE FROM rw_view1 WHERE a=5;
          Index Cond: ((a > 0) AND (a = 5))
 (3 rows)
 
+-- it's still updatable if we add a DO ALSO rule
+CREATE TABLE base_tbl_hist(ts timestamptz default now(), a int, b text);
+CREATE RULE base_tbl_log AS ON INSERT TO rw_view1 DO ALSO
+  INSERT INTO base_tbl_hist(a,b) VALUES(new.a, new.b);
+SELECT table_name, is_updatable, is_insertable_into
+  FROM information_schema.views
+ WHERE table_name = 'rw_view1';
+ table_name | is_updatable | is_insertable_into 
+------------+--------------+--------------------
+ rw_view1   | YES          | YES
+(1 row)
+
+-- Check behavior with DEFAULTs (bug #17633)
+INSERT INTO rw_view1 VALUES (9, DEFAULT), (10, DEFAULT);
+SELECT a, b FROM base_tbl_hist;
+ a  | b 
+----+---
+  9 | 
+ 10 | 
+(2 rows)
+
 DROP TABLE base_tbl CASCADE;
 NOTICE:  drop cascades to view rw_view1
+DROP TABLE base_tbl_hist;
 -- view on top of view
 CREATE TABLE base_tbl (a int PRIMARY KEY, b text DEFAULT 'Unspecified');
 INSERT INTO base_tbl SELECT i, 'Row ' || i FROM generate_series(-2, 2) g(i);
index fa206a8fe7e739f7e632cc3ff34e5564a19e3dd6..a6aebbc1303511b6d34cb53c5c9f5fa72d9bad60 100644 (file)
@@ -148,7 +148,24 @@ SELECT * FROM base_tbl;
 EXPLAIN (costs off) UPDATE rw_view1 SET a=6 WHERE a=5;
 EXPLAIN (costs off) DELETE FROM rw_view1 WHERE a=5;
 
+-- it's still updatable if we add a DO ALSO rule
+
+CREATE TABLE base_tbl_hist(ts timestamptz default now(), a int, b text);
+
+CREATE RULE base_tbl_log AS ON INSERT TO rw_view1 DO ALSO
+  INSERT INTO base_tbl_hist(a,b) VALUES(new.a, new.b);
+
+SELECT table_name, is_updatable, is_insertable_into
+  FROM information_schema.views
+ WHERE table_name = 'rw_view1';
+
+-- Check behavior with DEFAULTs (bug #17633)
+
+INSERT INTO rw_view1 VALUES (9, DEFAULT), (10, DEFAULT);
+SELECT a, b FROM base_tbl_hist;
+
 DROP TABLE base_tbl CASCADE;
+DROP TABLE base_tbl_hist;
 
 -- view on top of view