Horton

Thread

Commits

  1. Make rewriter prevent auto-updates on views with conditional INSTEAD rules.

  2. Allow only some columns of a view to be auto-updateable.

  1. Errors when update a view with conditional-INSTEAD rules

    Pengzhou Tang <ptang@pivotal.io> — 2019-12-03T11:05:34Z

    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

  2. Re: Errors when update a view with conditional-INSTEAD rules

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-01-04T16:08:19Z

    On Tue, 3 Dec 2019 at 11:06, Pengzhou Tang <ptang@pivotal.io> 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

  3. Re: Errors when update a view with conditional-INSTEAD rules

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-04T17:13:44Z

    Dean Rasheed <dean.a.rasheed@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.

			regards, tom lane

  4. Re: Errors when update a view with conditional-INSTEAD rules

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-01-04T18:12:13Z

    On Sat, 4 Jan 2020 at 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rasheed@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.
>

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

  5. Re: Errors when update a view with conditional-INSTEAD rules

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-01-07T11:00:30Z

    On Sat, 4 Jan 2020 at 18:12, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Sat, 4 Jan 2020 at 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Dean Rasheed <dean.a.rasheed@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

  6. Re: Errors when update a view with conditional-INSTEAD rules

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-01-14T10:18:38Z

    On Tue, 7 Jan 2020 at 11:00, Dean Rasheed <dean.a.rasheed@gmail.com> 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

  7. Re: Errors when update a view with conditional-INSTEAD rules

    Pengzhou Tang <ptang@pivotal.io> — 2020-01-17T06:13:59Z

    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 <dean.a.rasheed@gmail.com>
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

  8. Re: Errors when update a view with conditional-INSTEAD rules

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-01-17T14:36:15Z

    On Fri, 17 Jan 2020 at 06:14, Pengzhou Tang <ptang@pivotal.io> 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

  9. Re: Errors when update a view with conditional-INSTEAD rules

    Pengzhou Tang <ptang@pivotal.io> — 2020-01-22T06:28:01Z

    > 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.