Re: WIP: Triggers on VIEWs

On 5 October 2010 21:17, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
> Basic summary of this patch:
>

Thanks for the review.

> * The patch includes a fairly complete discussion about INSTEAD OF triggers
> and their usage on views. There are also additional enhancements to the RULE
> documentation, which seems, given that this might supersede the usage of
> RULES for updatable views, reasonable.
>
> * The patch passes regressions tests and comes with a bunch of its own
> regression tests. I think they are complete, they cover statement and row
> Level trigger and show the usage for JOINed views for example.
>
> * I've checked against a draft of the SQL standard, the behavior of the
> patch seems to match the spec (my copy might be out of date, however).
>
> * The code looks pretty good to me, there are some low level error messages
> exposing some implementation details, which could be changed (e.g. "wholerow
> is NULL"), but given that this is most of the time unexpected and is used in
> some older code as well, this doesn't seem very important.
>

Hopefully that error should never happen, since it would indicate a
bug in the code rather than a user error.

> * The implementation introduces the notion of "wholerow". This is a junk
> target list entry which allows the executor to carry the view information to
> an INSTEAD OF trigger. In case of DELETE/UPDATE, the rewriter is responsible
> to inject the new "wholerow" TLE and make the original query to point to a
> new range table entry (correct me, when i'm wrong), which is based on the
> view's query. I'm not sure i'm happy with the notion of "wholerow" here,
> maybe "viewrow" or "viewtarget" is more descriptive?
>

That's a good description of how it works. I chose "wholerow" because
that matched similar terminology used already, for example in
preptlist.c when doing FOR UPDATE/SHARE on a view. I don't feel
strongly about it, but my inclination is to stick with "wholerow"
unless someone feels strongly otherwise.

> * I'm inclined to say that INSTEAD OF triggers have less overhead than
> RULES, but this is not proven yet with a reasonable benchmark.
>

It's difficult to come up with a general statement on performance
because there are so many variables that might affect it. In a few
simple tests, I found that for repeated small updates RULEs and
TRIGGERs perform roughly the same, but for bulk updates (one query
updating 1000s of rows) a RULE is best.

> I would like to do some more tests/review, but going to mark this patch as
> "Ready for Committer", so that someone more qualified on the executor part
> can have a look on it during this commitfest, if that's okay for us?
>

Thanks for looking at it. I hope this is useful functionality to make
it easier to write updatable views, and perhaps it will help with
implementing auto-updatable views too.

Cheers,
Dean

> --
> Thanks
>
>        Bernd
>