Thread: BUG #18546: Attempt to insert default into a non-updatable column of a view fails with a dubious error

The following bug has been logged on the website:

Bug reference:      18546
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 17beta2
Operating system:   Ubuntu 22.04
Description:

The following query:
CREATE TABLE t (a int);
CREATE VIEW v AS  SELECT a, a + 0 AS a0 FROM t;
INSERT INTO v values (default, default);
raises
ERROR:  XX000: attribute number 2 not found in view targetlist
LOCATION:  adjust_view_column_set, rewriteHandler.c:3045

Whilst
INSERT INTO v values (default, 1);
fails with clearer
ERROR:  0A000: cannot insert into column "a0" of view "v"
DETAIL:  View columns that are not columns of their base relation are not
updatable.

PG Bug reporting form <noreply@postgresql.org> writes:
> The following query:
> CREATE TABLE t (a int);
> CREATE VIEW v AS  SELECT a, a + 0 AS a0 FROM t;
> INSERT INTO v values (default, default);
> raises
> ERROR:  XX000: attribute number 2 not found in view targetlist
> LOCATION:  adjust_view_column_set, rewriteHandler.c:3045

> Whilst
> INSERT INTO v values (default, 1);
> fails with clearer
> ERROR:  0A000: cannot insert into column "a0" of view "v"
> DETAIL:  View columns that are not columns of their base relation are not
> updatable.

Interesting.  I thought this was a wrong-order-of-checks problem,
but it's more subtle than that.  The "cannot insert into column "a0""
message is produced when view_cols_are_auto_updatable recognizes that
we can't assign to that particular column.  But
view_cols_are_auto_updatable doesn't test the a0 column, because
it's told to check the columns that are targeted by the query
targetlist after rewriteTargetListIU ... and rewriteTargetListIU has
thrown away the DEFAULT markers, on the grounds that the column
defaults are null and we don't need to represent that explicitly.

The existing code/comments (dating AFAICS to Dean's cab5dc5da)
already point out that rewriteTargetListIU can add targetlist items,
but we missed the fact that it can delete them too.  So it seems like
what we need to do is union the original set of target columns with
what's listed in the targetlist, as attached.  I suppose we could
also rethink the decision to throw away null defaults, but that seems
much more invasive.

Thanks for the report!

            regards, tom lane

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 8a29fbbc46..e1d805d113 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2986,7 +2986,7 @@ relation_is_updatable(Oid reloid,
  *
  * This is used with simply-updatable views to map column-permissions sets for
  * the view columns onto the matching columns in the underlying base relation.
- * The targetlist is expected to be a list of plain Vars of the underlying
+ * Relevant entries in the targetlist must be plain Vars of the underlying
  * relation (as per the checks above in view_query_is_auto_updatable).
  */
 static Bitmapset *
@@ -3186,6 +3186,10 @@ rewriteTargetView(Query *parsetree, Relation view)
      */
     viewquery = copyObject(get_view_query(view));

+    /* Locate RTE and perminfo describing the view in the outer query */
+    view_rte = rt_fetch(parsetree->resultRelation, parsetree->rtable);
+    view_perminfo = getRTEPermissionInfo(parsetree->rteperminfos, view_rte);
+
     /*
      * Are we doing INSERT/UPDATE, or MERGE containing INSERT/UPDATE?  If so,
      * various additional checks on the view columns need to be applied, and
@@ -3225,17 +3229,26 @@ rewriteTargetView(Query *parsetree, Relation view)

     /*
      * For INSERT/UPDATE (or MERGE containing INSERT/UPDATE) the modified
-     * columns must all be updatable. Note that we get the modified columns
-     * from the query's targetlist, not from the result RTE's insertedCols
-     * and/or updatedCols set, since rewriteTargetListIU may have added
-     * additional targetlist entries for view defaults, and these must also be
-     * updatable.
+     * columns must all be updatable.
      */
     if (insert_or_update)
     {
-        Bitmapset  *modified_cols = NULL;
+        Bitmapset  *modified_cols;
         char       *non_updatable_col;

+        /*
+         * Compute the set of modified columns as those listed in the result
+         * RTE's insertedCols and/or updatedCols sets plus those that are
+         * targets of the query's targetlist(s).  We must consider the query's
+         * targetlist because rewriteTargetListIU may have added additional
+         * targetlist entries for view defaults, and these must also be
+         * updatable.  But rewriteTargetListIU can also remove entries if they
+         * are DEFAULT markers and the column's default is NULL, so
+         * considering only the targetlist would also be wrong.
+         */
+        modified_cols = bms_union(view_perminfo->insertedCols,
+                                  view_perminfo->updatedCols);
+
         foreach(lc, parsetree->targetList)
         {
             TargetEntry *tle = (TargetEntry *) lfirst(lc);
@@ -3337,9 +3350,6 @@ rewriteTargetView(Query *parsetree, Relation view)
         }
     }

-    /* Locate RTE describing the view in the outer query */
-    view_rte = rt_fetch(parsetree->resultRelation, parsetree->rtable);
-
     /*
      * If we get here, view_query_is_auto_updatable() has verified that the
      * view contains a single base relation.
@@ -3434,10 +3444,7 @@ rewriteTargetView(Query *parsetree, Relation view)
      * Note: the original view's RTEPermissionInfo remains in the query's
      * rteperminfos so that the executor still performs appropriate
      * permissions checks for the query caller's use of the view.
-     */
-    view_perminfo = getRTEPermissionInfo(parsetree->rteperminfos, view_rte);
-
-    /*
+     *
      * Disregard the perminfo in viewquery->rteperminfos that the base_rte
      * would currently be pointing at, because we'd like it to point now to a
      * new one that will be filled below.  Must set perminfoindex to 0 to not
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 44aba0d1dc..420769a40c 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2101,6 +2101,9 @@ DETAIL:  View columns that refer to system columns are not updatable.
 INSERT INTO rw_view1 (s, c, a) VALUES (null, null, 1.1); -- should fail
 ERROR:  cannot insert into column "s" of view "rw_view1"
 DETAIL:  View columns that are not columns of their base relation are not updatable.
+INSERT INTO rw_view1 (s, c, a) VALUES (default, default, 1.1); -- should fail
+ERROR:  cannot insert into column "s" of view "rw_view1"
+DETAIL:  View columns that are not columns of their base relation are not updatable.
 INSERT INTO rw_view1 (a) VALUES (1.1) RETURNING a, s, c; -- OK
   a  |         s         |         c
 -----+-------------------+-------------------
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index abfa5574a6..93b693ae83 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1111,6 +1111,7 @@ CREATE VIEW rw_view1 AS

 INSERT INTO rw_view1 VALUES (null, null, 1.1, null); -- should fail
 INSERT INTO rw_view1 (s, c, a) VALUES (null, null, 1.1); -- should fail
+INSERT INTO rw_view1 (s, c, a) VALUES (default, default, 1.1); -- should fail
 INSERT INTO rw_view1 (a) VALUES (1.1) RETURNING a, s, c; -- OK
 UPDATE rw_view1 SET s = s WHERE a = 1.1; -- should fail
 UPDATE rw_view1 SET a = 1.05 WHERE a = 1.1 RETURNING s; -- OK

On Sat, 20 Jul 2024 at 17:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> The existing code/comments (dating AFAICS to Dean's cab5dc5da)
> already point out that rewriteTargetListIU can add targetlist items,
> but we missed the fact that it can delete them too.  So it seems like
> what we need to do is union the original set of target columns with
> what's listed in the targetlist, as attached.
>

Ah yes, that makes sense and the fix looks good. Thanks for taking care of that.

I had always thought that rewriteTargetListIU() only ever added or
merged items, somehow overlooking the fact that it could also delete
them. In my defence, it's very easy to get that impression just by
reading the function's comments. I think it's worth updating those
comments to mention that. Something like the attached, perhaps.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> I had always thought that rewriteTargetListIU() only ever added or
> merged items, somehow overlooking the fact that it could also delete
> them. In my defence, it's very easy to get that impression just by
> reading the function's comments. I think it's worth updating those
> comments to mention that. Something like the attached, perhaps.

No objection here.

            regards, tom lane