Re: Errors when update a view with conditional-INSTEAD rules
> I am wondering whether a simple auto-updatable view can have a > conditional update instead rule. > > Well, the decision reached in [1] was that we wouldn't allow that. We > could decide to allow it now as a new feature enhancement, but it > wouldn't be a back-patchable bug-fix, and to be honest I wouldn't be > particularly excited about adding such a feature now. We already get > enough reports related to multiple rule actions behaving in > counter-intuitive ways that trip up users. I don't think we should be > enhancing the rule system, but rather encouraging users not to use it > and use triggers instead. > > Ok, that makes sense, thanks for the explanation.
Re: Errors when update a view with conditional-INSTEAD rules
On Fri, 17 Jan 2020 at 06:14, Pengzhou Tang wrote: > > I am wondering whether a simple auto-updatable view can have a conditional > update instead rule. Well, the decision reached in [1] was that we wouldn't allow that. We could decide to allow it now as a new feature enhancement, but it wouldn't be a back-patchable bug-fix, and to be honest I wouldn't be particularly excited about adding such a feature now. We already get enough reports related to multiple rule actions behaving in counter-intuitive ways that trip up users. I don't think we should be enhancing the rule system, but rather encouraging users not to use it and use triggers instead. > The document also says that: > "There is a catch if you try to use conditional rules for complex view > updates: there must be an unconditional > INSTEAD rule for each action you wish to allow on the view" which makes me > think a simple view can have a > conditional INSTEAD rule. And the document is explicitly changed in commit > a99c42f291421572aef2: > - There is a catch if you try to use conditional rules for view > + There is a catch if you try to use conditional rules for complex view > > Does that mean we should support conditional rules for a simple view? > No. I don't recall why that wording was changed in that commit, but I think it's meant to be read as "complex updates on views" -- i.e., the word "complex" refers to the complexity of the update logic, not the complexity of the view. Nothing in that paragraph is related to complex vs simple views, it's about complex sets of rules. Regards, Dean [1] https://www.postgresql.org/message-id/25777.1352325888%40sss.pgh.pa.us
Re: Errors when update a view with conditional-INSTEAD rules
Thanks a lot, Dean, to look into this and also sorry for the late reply. On Sun, Jan 5, 2020 at 12:08 AM Dean Rasheed wrote: > Tracing it through, this seems to be a result of > cab5dc5daf2f6f5da0ce79deb399633b4bb443b5 which added support for > updatable views with a mix of updatable and non-updatable columns. > 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. But > when the planner bulds the simple_rel_array, it only adds entries for > relations referenced in the query's jointree, which only includes the > base table by this point, not the view. Thus the new targetlist entry > added by expand_targetlist() refers to a NULL slot in the > simple_rel_array, and it blows up. > > That's a great analysis of this issue. > Given that this is a query that's going to fail anyway, I'm inclined > to think that the right thing to do is to throw the error sooner, in > rewriteQuery(), rather than attempting to plan a query that cannot be > executed. > I am wondering whether a simple auto-updatable view can have a conditional update instead rule. For the test case I added, does bellow plan looks reasonable? gpadmin=# explain UPDATE v1 SET b = 2 WHERE a = 1; QUERY PLAN --- Insert on t2 (cost=0.00..49.55 rows=1 width=8) -> Seq Scan on t1 (cost=0.00..49.55 rows=1 width=8) Filter: ((b > 100) AND (a > 2) AND (a = 1)) Update on t1 (cost=0.00..49.55 rows=3 width=14) -> Seq Scan on t1 (cost=0.00..49.55 rows=3 width=14) Filter: (((a > 2) IS NOT TRUE) AND (b > 100) AND (a = 1)) (7 rows) gpadmin=# UPDATE v1 SET b = 2 WHERE a = 1; UPDATE 1 The document also says that: "There is a catch if you try to use conditional rules for *complex view* updates: there must be an unconditional INSTEAD rule for each action you wish to allow on the view" which makes me think a simple view can have a conditional INSTEAD rule. And the document is explicitly changed in commit a99c42f291421572aef2: - There is a catch if you try to use conditional rules for view + There is a catch if you try to use conditional rules for complex view Does that mean we should support conditional rules for a simple view? Regards, Pengzhou Tang
Re: Errors when update a view with conditional-INSTEAD rules
On Tue, 7 Jan 2020 at 11:00, Dean Rasheed wrote: > > 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). > [finally getting back to this] Hearing no objections, I have pushed and back-patched this. Regards, Dean
Re: Errors when update a view with conditional-INSTEAD rules
On Sat, 4 Jan 2020 at 18:12, Dean Rasheed wrote:
>
> On Sat, 4 Jan 2020 at 17:13, Tom Lane wrote:
> >
> > Dean Rasheed 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
Date: Tue, 7 Jan 2020 09:17:12 +
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,
Re: Errors when update a view with conditional-INSTEAD rules
On Sat, 4 Jan 2020 at 17:13, Tom Lane wrote:
>
> Dean Rasheed 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.
>
Perhaps it was more to do with history and not wanting to duplicate
code. Before we had auto-updatable views, it was always the executor
that threw this error. With the addition of auto-updatable views, we
also throw the error from rewriteTargetView() if there are no rules or
triggers. But there is a difference -- rewriteTargetView() has more
detailed information about why the view isn't auto-updatable, which it
includes in the error detail.
I think that the required information is easily available in the
rewriter though. Currently RewriteQuery() is doing this:
if ( !instead // No unconditional INSTEAD rules
&& qual_product == NULL // No conditional INSTEAD rules either
&& relkind == VIEW
&& !view_has_instead_trigger() )
{
// Attempt auto-update, throwing an error if not possible
rewriteTargetView(...)
...
}
So if that were to become something like:
if ( !instead // No unconditional INSTEAD rules
&& relkind == VIEW
&& !view_has_instead_trigger() )
{
if (qual_product != NULL)
{
// Conditional INSTEAD rules exist, but no unconditional INSTEAD rules
// or INSTEAD OF triggers, so throw an error
...
}
// Attempt auto-update, throwing an error if not possible
rewriteTargetView(...)
...
}
then in theory I think the error condition in the executor should
never be triggered. That will lead to a few lines of duplicated code
because the error-throwing code block includes a switch on command
type. However, it also gives us an opportunity to be a more specific
in the new error, with detail for this specific case.
Regards,
Dean
Re: Errors when update a view with conditional-INSTEAD rules
Dean Rasheed 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. regards, tom lane
Re: Errors when update a view with conditional-INSTEAD rules
On Tue, 3 Dec 2019 at 11:06, Pengzhou Tang wrote: > > Hi Hackers, > > I hit an error when updating a view with conditional INSTEAD OF rules, the > reproduce steps are list below: > > CREATE TABLE t1(a int, b int); > CREATE TABLE t2(a int, b int); > CREATE VIEW v1 AS SELECT * FROM t1 where b > 100; > > INSERT INTO v1 values(1, 110); > SELECT * FROM t1; > > CREATE OR REPLACE rule r1 AS > ON UPDATE TO v1 > WHERE old.a > new.b > DO INSTEAD ( > INSERT INTO t2 values(old.a, old.b); > ); > > UPDATE v1 SET b = 2 WHERE a = 1; > > ERROR: no relation entry for relid 2 > I took a look at this and one thing that's clear is that it should not be producing that error. Testing that case in 9.3, where updatable views were first added, it produces the expected error: ERROR: cannot update view "v1" HINT: To enable updating the view, provide an INSTEAD OF UPDATE trigger or an unconditional ON UPDATE DO INSTEAD rule. That is the intended behaviour -- see [1] and the discussion that followed. Basically the presence of INSTEAD triggers or INSTEAD rules (conditional or otherwise) disables auto-updates. If you have any conditional INSTEAD rules, you must also have an unconditional INSTEAD rule or INSTEAD OF trigger to make the view updatable. So what's curious is why this test case now produces this rather uninformative error: ERROR: no relation entry for relid 2 which really shouldn't be happening. Tracing it through, this seems to be a result of cab5dc5daf2f6f5da0ce79deb399633b4bb443b5 which added support for updatable views with a mix of updatable and non-updatable columns. 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. But when the planner bulds the simple_rel_array, it only adds entries for relations referenced in the query's jointree, which only includes the base table by this point, not the view. Thus the new targetlist entry added by expand_targetlist() refers to a NULL slot in the simple_rel_array, and it blows up. Given that this is a query that's going to fail anyway, I'm inclined to think that the right thing to do is to throw the error sooner, in rewriteQuery(), rather than attempting to plan a query that cannot be executed. Thoughts? Regards, Dean [1] https://www.postgresql.org/message-id/25777.1352325888%40sss.pgh.pa.us
Errors when update a view with conditional-INSTEAD rules
Hi Hackers, I hit an error when updating a view with conditional INSTEAD OF rules, the reproduce steps are list below: CREATE TABLE t1(a int, b int); CREATE TABLE t2(a int, b int); CREATE VIEW v1 AS SELECT * FROM t1 where b > 100; INSERT INTO v1 values(1, 110); SELECT * FROM t1; CREATE OR REPLACE rule r1 AS ON UPDATE TO v1 WHERE old.a > new.b DO INSTEAD ( INSERT INTO t2 values(old.a, old.b); ); UPDATE v1 SET b = 2 WHERE a = 1; *ERROR: no relation entry for relid 2* With some hacks, It is because, for conditional INSTEAD OF rules conditional, the original UPDATE operation also need to perform on the view, however, we didn't rewrite the target view for any view with INSTEAD rules. There should be only two cases that you can skip the rewrite of target view: 1) the view has INSTEAD OF triggers on the operations, the operations will be replaced by trigger-defined 2) the view has INSTEAD OF rules and it is non conditional rules, the operations will be replaced by actions. It should be a typo in commit a99c42f291421572aef2, there is a description in documents: "There is a catch if you try to use conditional rules for complex view updates: there must be an unconditional INSTEAD rule for each action you wish to allow on the view." Commit a99c42f291421572aef2 explicitly change the description that the restriction only applies to complex view, conditional INSTEAD rule should work for a simple view. I attached a patch to fix it, please take a look, Thanks, Pengzhou 0001-Rewrite-the-target-view-if-it-has-conditional-INSTEA.patch Description: Binary data
