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

On Tue, 3 Dec 2019 at 11:06, Pengzhou Tang <ptang(at)pivotal(dot)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