On Sat, 4 Jan 2020 at 18:12, Dean Rasheed <dean.a.rash...@gmail.com> wrote: > > On Sat, 4 Jan 2020 at 17:13, Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Dean Rasheed <dean.a.rash...@gmail.com> writes: > > > That included a change to rewriteTargetListIU() to prevent it from > > > adding dummy targetlist entries for unassigned-to attributes for > > > auto-updatable views, in case they are no longer simple references to > > > the underlying relation. Instead, that is left to expand_targetlist(), > > > as for a normal table. However, in this case (an UPDATE on a view with > > > a conditional rule), the target relation of the original query isn't > > > rewritten (we leave it to the executor to report the error), and so > > > expand_targetlist() ends up adding a new targetlist entry that > > > references the target relation, which is still the original view. > > > > So why did we leave it to the executor to throw an error? I have > > a feeling it was either because the rewriter didn't have (easy?) > > access to the info, or it seemed like it'd be duplicating code. > > > I think that the required information is easily available in the > rewriter ...
Here's a patch along those lines. Yes, it's a little more code duplication, but I think it's worth it for the more detailed error. There was no previous regression test coverage of this case so I added some (all other test output is unaltered). The existing comment in the executor check clearly implied that it thought that error was unreachable there, and I think it now is, but it seems worth leaving it just in case. Regards, Dean
From e9036832192b27b11f1beff5871618f349477819 Mon Sep 17 00:00:00 2001 From: Dean Rasheed <dean.a.rash...@gmail.com> Date: Tue, 7 Jan 2020 09:17:12 +0000 Subject: [PATCH] Make rewriter prevent auto-updates on views with conditional INSTEAD rules. A view with conditional INSTEAD rules and no unconditional INSTEAD rules or INSTEAD OF triggers is not auto-updatable. Previously we relied on a check in the executor to catch this, but that's problematic since the planner may fail to properly handle such a query and thus return a particularly unhelpful error to the user. Instead, trap this in the rewriter and report the correct error there. Doing so also allows us to include more useful error detail than the executor check can provide. Per report from Pengzhou Tang. Back-patch to all supported branches. Discussion: https://postgr.es/m/CAG4reAQn+4xB6xHJqWdtE0ve_WqJkdyCV4P=tryr4kn8_3_...@mail.gmail.com --- src/backend/executor/execMain.c | 8 ++-- src/backend/rewrite/rewriteHandler.c | 60 ++++++++++++++++++++++++--- src/test/regress/expected/updatable_views.out | 21 ++++++++++ src/test/regress/sql/updatable_views.sql | 14 +++++++ 4 files changed, 94 insertions(+), 9 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 4181a7e343..b03e02ae6c 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1101,10 +1101,10 @@ CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation) /* * Okay only if there's a suitable INSTEAD OF trigger. Messages - * here should match rewriteHandler.c's rewriteTargetView, except - * that we omit errdetail because we haven't got the information - * handy (and given that we really shouldn't get here anyway, it's - * not worth great exertion to get). + * here should match rewriteHandler.c's rewriteTargetView and + * RewriteQuery, except that we omit errdetail because we haven't + * got the information handy (and given that we really shouldn't + * get here anyway, it's not worth great exertion to get). */ switch (operation) { diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index e9fefec8b3..3b4f28874a 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -3670,21 +3670,71 @@ RewriteQuery(Query *parsetree, List *rewrite_events) } /* - * If there were no INSTEAD rules, and the target relation is a view - * without any INSTEAD OF triggers, see if the view can be + * If there was no unqualified INSTEAD rule, and the target relation + * is a view without any INSTEAD OF triggers, see if the view can be * automatically updated. If so, we perform the necessary query * transformation here and add the resulting query to the * product_queries list, so that it gets recursively rewritten if * necessary. + * + * If the view cannot be automatically updated, we throw an error here + * which is OK since the query would fail at runtime anyway. Throwing + * the error here is preferable to the executor check since we have + * more detailed information available about why the view isn't + * updatable. */ - if (!instead && qual_product == NULL && + if (!instead && rt_entry_relation->rd_rel->relkind == RELKIND_VIEW && !view_has_instead_trigger(rt_entry_relation, event)) { /* + * If there were any qualified INSTEAD rules, don't allow the view + * to be automatically updated (an unqualified INSTEAD rule or + * INSTEAD OF trigger is required). + * + * The messages here should match execMain.c's CheckValidResultRel + * and in principle make those checks in executor unnecessary, but + * we keep them just in case. + */ + if (qual_product != NULL) + { + switch (parsetree->commandType) + { + case CMD_INSERT: + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot insert into view \"%s\"", + RelationGetRelationName(rt_entry_relation)), + errdetail("Views with conditional DO INSTEAD rules are not automatically updatable."), + errhint("To enable inserting into the view, provide an INSTEAD OF INSERT trigger or an unconditional ON INSERT DO INSTEAD rule."))); + break; + case CMD_UPDATE: + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot update view \"%s\"", + RelationGetRelationName(rt_entry_relation)), + errdetail("Views with conditional DO INSTEAD rules are not automatically updatable."), + errhint("To enable updating the view, provide an INSTEAD OF UPDATE trigger or an unconditional ON UPDATE DO INSTEAD rule."))); + break; + case CMD_DELETE: + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot delete from view \"%s\"", + RelationGetRelationName(rt_entry_relation)), + errdetail("Views with conditional DO INSTEAD rules are not automatically updatable."), + errhint("To enable deleting from the view, provide an INSTEAD OF DELETE trigger or an unconditional ON DELETE DO INSTEAD rule."))); + break; + default: + elog(ERROR, "unrecognized CmdType: %d", + (int) parsetree->commandType); + break; + } + } + + /* + * Attempt to rewrite the query to automatically update the view. * This throws an error if the view can't be automatically - * updated, but that's OK since the query would fail at runtime - * anyway. + * updated. */ parsetree = rewriteTargetView(parsetree, rt_entry_relation); diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out index d54a0a1c2e..5de53f2782 100644 --- a/src/test/regress/expected/updatable_views.out +++ b/src/test/regress/expected/updatable_views.out @@ -330,6 +330,27 @@ UPDATE ro_view20 SET b=upper(b); ERROR: cannot update view "ro_view20" DETAIL: Views that return set-returning functions are not automatically updatable. HINT: To enable updating the view, provide an INSTEAD OF UPDATE trigger or an unconditional ON UPDATE DO INSTEAD rule. +-- A view with a conditional INSTEAD rule but no unconditional INSTEAD rules +-- or INSTEAD OF triggers should be non-updatable and generate useful error +-- messages with appropriate detail +CREATE RULE rw_view16_ins_rule AS ON INSERT TO rw_view16 + WHERE NEW.a > 0 DO INSTEAD INSERT INTO base_tbl VALUES (NEW.a, NEW.b); +CREATE RULE rw_view16_upd_rule AS ON UPDATE TO rw_view16 + WHERE OLD.a > 0 DO INSTEAD UPDATE base_tbl SET b=NEW.b WHERE a=OLD.a; +CREATE RULE rw_view16_del_rule AS ON DELETE TO rw_view16 + WHERE OLD.a > 0 DO INSTEAD DELETE FROM base_tbl WHERE a=OLD.a; +INSERT INTO rw_view16 (a, b) VALUES (3, 'Row 3'); -- should fail +ERROR: cannot insert into view "rw_view16" +DETAIL: Views with conditional DO INSTEAD rules are not automatically updatable. +HINT: To enable inserting into the view, provide an INSTEAD OF INSERT trigger or an unconditional ON INSERT DO INSTEAD rule. +UPDATE rw_view16 SET b='ROW 2' WHERE a=2; -- should fail +ERROR: cannot update view "rw_view16" +DETAIL: Views with conditional DO INSTEAD rules are not automatically updatable. +HINT: To enable updating the view, provide an INSTEAD OF UPDATE trigger or an unconditional ON UPDATE DO INSTEAD rule. +DELETE FROM rw_view16 WHERE a=2; -- should fail +ERROR: cannot delete from view "rw_view16" +DETAIL: Views with conditional DO INSTEAD rules are not automatically updatable. +HINT: To enable deleting from the view, provide an INSTEAD OF DELETE trigger or an unconditional ON DELETE DO INSTEAD rule. DROP TABLE base_tbl CASCADE; NOTICE: drop cascades to 16 other objects DETAIL: drop cascades to view ro_view1 diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql index e50a4c52ee..64f23d0902 100644 --- a/src/test/regress/sql/updatable_views.sql +++ b/src/test/regress/sql/updatable_views.sql @@ -101,6 +101,20 @@ DELETE FROM ro_view18; UPDATE ro_view19 SET last_value=1000; UPDATE ro_view20 SET b=upper(b); +-- A view with a conditional INSTEAD rule but no unconditional INSTEAD rules +-- or INSTEAD OF triggers should be non-updatable and generate useful error +-- messages with appropriate detail +CREATE RULE rw_view16_ins_rule AS ON INSERT TO rw_view16 + WHERE NEW.a > 0 DO INSTEAD INSERT INTO base_tbl VALUES (NEW.a, NEW.b); +CREATE RULE rw_view16_upd_rule AS ON UPDATE TO rw_view16 + WHERE OLD.a > 0 DO INSTEAD UPDATE base_tbl SET b=NEW.b WHERE a=OLD.a; +CREATE RULE rw_view16_del_rule AS ON DELETE TO rw_view16 + WHERE OLD.a > 0 DO INSTEAD DELETE FROM base_tbl WHERE a=OLD.a; + +INSERT INTO rw_view16 (a, b) VALUES (3, 'Row 3'); -- should fail +UPDATE rw_view16 SET b='ROW 2' WHERE a=2; -- should fail +DELETE FROM rw_view16 WHERE a=2; -- should fail + DROP TABLE base_tbl CASCADE; DROP VIEW ro_view10, ro_view12, ro_view18; DROP SEQUENCE uv_seq CASCADE; -- 2.16.4
