Обсуждение: Errors when update a view with conditional-INSTEAD rules

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

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

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

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

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

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