pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18546: Attempt to insert default into a non-updatable column of a view fails with a dubious error
4+ messages / 3 participants
[nested] [flat]
* BUG #18546: Attempt to insert default into a non-updatable column of a view fails with a dubious error
@ 2024-07-20 09:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: PG Bug reporting form @ 2024-07-20 09:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
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.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18546: Attempt to insert default into a non-updatable column of a view fails with a dubious error
@ 2024-07-20 16:26 Tom Lane <tgl@sss.pgh.pa.us>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: Tom Lane @ 2024-07-20 16:26 UTC (permalink / raw)
To: exclusion@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org, Dean Rasheed <dean.a.rasheed@gmail.com>
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
Attachments:
[text/x-diff] fix-bug-18546.patch (5.0K, ../../320614.1721492789@sss.pgh.pa.us/2-fix-bug-18546.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18546: Attempt to insert default into a non-updatable column of a view fails with a dubious error
@ 2024-07-22 08:41 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 4+ messages in thread
From: Dean Rasheed @ 2024-07-22 08:41 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
Attachments:
[text/x-patch] comment-update-for-bug-18546.patch (2.3K, ../../CAEZATCWLfKrE7m0pNMD3-5GCk=swfZWUMmatvHqDPMO9KNa3Vg@mail.gmail.com/2-comment-update-for-bug-18546.patch)
download | inline diff:
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index e1d805d..06f9742
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -724,11 +724,15 @@ adjustJoinTreeList(Query *parsetree, boo
* attributes that have defaults and are not assigned to in the given tlist.
* (We do not insert anything for default-less attributes, however. The
* planner will later insert NULLs for them, but there's no reason to slow
- * down rewriter processing with extra tlist nodes.) Also, for both INSERT
- * and UPDATE, replace explicit DEFAULT specifications with column default
- * expressions.
+ * down rewriter processing with extra tlist nodes. We also do not insert
+ * anything for stored generated columns; those are fixed up in the executor.)
*
- * 2. Merge multiple entries for the same target attribute, or declare error
+ * 2. For INSERT and UPDATE, process any set-to-DEFAULT tlist entries. Any
+ * entries for columns that have defaults are replaced with the column default
+ * expressions, and any entries for default-less or stored generated columns
+ * are removed (for the same reasons as above).
+ *
+ * 3. Merge multiple entries for the same target attribute, or declare error
* if we can't. Multiple entries are only allowed for INSERT/UPDATE of
* portions of an array or record field, for example
* UPDATE table SET foo[2] = 42, foo[4] = 43;
@@ -736,11 +740,11 @@ adjustJoinTreeList(Query *parsetree, boo
* the expression we want to produce in this case is like
* foo = array_set_element(array_set_element(foo, 2, 42), 4, 43)
*
- * 3. Sort the tlist into standard order: non-junk fields in order by resno,
+ * 4. Sort the tlist into standard order: non-junk fields in order by resno,
* then junk fields (these in no particular order).
*
- * We must do items 1 and 2 before firing rewrite rules, else rewritten
- * references to NEW.foo will produce wrong or incomplete results. Item 3
+ * We must do items 1, 2, and 3 before firing rewrite rules, else rewritten
+ * references to NEW.foo will produce wrong or incomplete results. Item 4
* is not needed for rewriting, but it is helpful for the planner, and we
* can do it essentially for free while handling the other items.
*
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18546: Attempt to insert default into a non-updatable column of a view fails with a dubious error
@ 2024-07-22 14:31 Tom Lane <tgl@sss.pgh.pa.us>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 0 replies; 4+ messages in thread
From: Tom Lane @ 2024-07-22 14:31 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-07-22 14:31 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-07-20 09:00 BUG #18546: Attempt to insert default into a non-updatable column of a view fails with a dubious error PG Bug reporting form <noreply@postgresql.org>
2024-07-20 16:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-22 08:41 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2024-07-22 14:31 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox