Re: Proof of concept: auto updatable views [Review of Patch] - Mailing list pgsql-hackers

On 8 November 2012 03:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> On 7 November 2012 22:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> This seems to me to be dangerous and unintuitive, not to mention
>>> underdocumented.  I think it would be better to just not do anything if
>>> there is any INSTEAD rule, period.
>
>> Is this any more dangerous than what already happens with qualified rules?
>
>> If we did nothing here then it would go on to either fire any INSTEAD
>> OF triggers or raise an error if there aren't any. The problem with
>> that is that it makes trigger-updatable views and auto-updatable views
>> inconsistent in their behaviour with qualified INSTEAD rules.
>
> Well, as submitted it's already pretty thoroughly inconsistent.  The way
> the existing code works is that if there's no INSTEAD rule, and there's
> no INSTEAD trigger, you get an error *at runtime*.  The reason for that
> is that the INSTEAD trigger might be added (or removed) between planning
> and execution.  This code tries to decide at plan time whether there's a
> relevant trigger, and that's just not very safe.
>
> I realize that you can't deliver the specific error messages that
> currently appear in view_is_auto_updatable if you don't throw the error
> at plan time.  But if you're going to claim that this ought to be
> consistent with the existing behavior, then I'm going to say we need to
> give that up and just have the runtime error, same as now.
>
> If you want the better error reporting (which I agree would be nice)
> then we need to revisit the interaction between INSTEAD triggers and
> INSTEAD rules anyway, and one of the things we probably should look at
> twice is whether it's sane at all to permit both a trigger and a
> qualified rule.  I'd bet long odds that nobody is using such a thing in
> the field, and I think disallowing it might be a good idea in order to
> disentangle these features a bit better.
>

OK, yes I think we do need to be throwing the error at runtime rather
than at plan time. That's pretty easy if we just keep the current
error message, but I think it would be very nice to have the more
specific DETAIL text to go along with the error.

We could save the value of is_view_auto_updatable() so that it's
available to the executor, but that seems very ugly. A better approach
might be to just call is_view_auto_updatable() again from the
executor. At the point where we would be calling it, we would already
know that the view isn't updatable, so we would just be looking for
friendlier DETAIL text to give to the user. There's a chance that the
view might have been changed structurally between planning an
execution, making that DETAIL text incorrect, or even changing the
fact that the view isn't updatable, but that seems pretty unlikely,
and no worse than similar risks with tables.

I think the whole thing with qualified rules is a separate issue. I
don't really have a strong opinion on it because I never use qualified
rules, but I am wary of changing the existing behaviour on
backwards-compatibility grounds. I don't much like the way qualified
rules work, but if we're going to support them then why should
trigger/auto-updatable views be an exception to the way they work?

Regards,
Dean