From: Tom Lane <tgl@sss.pgh.pa.us>
To: exclusion@gmail.com
cc: pgsql-bugs@lists.postgresql.org, Dean Rasheed <dean.a.rasheed@gmail.com>
Subject: Re: BUG #18546: Attempt to insert default into a non-updatable column
 of a view fails with a dubious error
In-reply-to: <18546-84a292e759a9361d@postgresql.org>
References: <18546-84a292e759a9361d@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Sat, 20 Jul 2024 09:00:01 -0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <320591.1721492772.0@sss.pgh.pa.us>
Date: Sat, 20 Jul 2024 12:26:29 -0400
Message-ID: <320614.1721492789@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/320614.1721492789%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <320591.1721492772.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-bug-18546.patch"; charset="us-ascii"
Content-ID: <320591.1721492772.2@sss.pgh.pa.us>
Content-Description: fix-bug-18546.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/re=
writeHandler.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 set=
s for
  * the view columns onto the matching columns in the underlying base rela=
tion.
- * The targetlist is expected to be a list of plain Vars of the underlyin=
g
+ * Relevant entries in the targetlist must be plain Vars of the underlyin=
g
  * relation (as per the checks above in view_query_is_auto_updatable).
  */
 static Bitmapset *
@@ -3186,6 +3186,10 @@ rewriteTargetView(Query *parsetree, Relation view)
 	 */
 	viewquery =3D copyObject(get_view_query(view));
 =

+	/* Locate RTE and perminfo describing the view in the outer query */
+	view_rte =3D rt_fetch(parsetree->resultRelation, parsetree->rtable);
+	view_perminfo =3D 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 =3D 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 =3D bms_union(view_perminfo->insertedCols,
+								  view_perminfo->updatedCols);
+
 		foreach(lc, parsetree->targetList)
 		{
 			TargetEntry *tle =3D (TargetEntry *) lfirst(lc);
@@ -3337,9 +3350,6 @@ rewriteTargetView(Query *parsetree, Relation view)
 		}
 	}
 =

-	/* Locate RTE describing the view in the outer query */
-	view_rte =3D 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 =3D 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 no=
t
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regr=
ess/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 a=
re 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/s=
ql/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 =3D s WHERE a =3D 1.1; -- should fail
 UPDATE rw_view1 SET a =3D 1.05 WHERE a =3D 1.1 RETURNING s; -- OK

------- =_aaaaaaaaaa0--