Re: WIP: Triggers on VIEWs

On 8 October 2010 16:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
>> 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?
>
> I've started looking at this patch now.  I think it would have been best
> submitted as two patches: one to add the SQL-spec "INSTEAD OF" trigger
> functionality, and a follow-on to extend INSTEAD OF triggers to views.

SQL-spec "INSTEAD OF" triggers *are* view triggers. I don't see how
you can separate the two.


> I'm thinking of breaking it apart into two separate commits along that
> line, mainly because I think INSTEAD OF is probably committable but
> I'm much less sure about the other part.
>
> In the INSTEAD OF part, the main gripe I've got is the data
> representation choice:
>
> ***************
> *** 1609,1614 ****
> --- 1609,1615 ----
>      List       *funcname;       /* qual. name of function to call */
>      List       *args;           /* list of (T_String) Values or NIL */
>      bool        before;         /* BEFORE/AFTER */
> +     bool        instead;        /* INSTEAD OF (overrides BEFORE/AFTER) */
>      bool        row;            /* ROW/STATEMENT */
>      /* events uses the TRIGGER_TYPE bits defined in catalog/pg_trigger.h */
>      int16       events;         /* INSERT/UPDATE/DELETE/TRUNCATE */
>
> This pretty much sucks, because not only is this a rather confusing
> definition, it's not really backwards compatible: any code that thinks
> "!before" must mean "after" is going to be silently broken.  Usually,
> when you do something that necessarily breaks old code, what you want
> is to make sure the breakage is obvious at compile time.  I think the
> right thing here is to replace "before" with a three-valued enum,
> perhaps called "timing", so as to force people to take another look
> at any code that touches the field directly.
>
> Although we already have macros TRIGGER_FIRED_AFTER/TRIGGER_FIRED_BEFORE
> that seem to mask the details here, the changes you had to make in
> contrib illustrate that the macros' callers could still be embedding this
> basic mistake of testing "!before" when they mean "after" or vice versa.
> I wonder whether we should intentionally rename the macros to force
> people to take another look at their logic.  Or is that going too far?
> Comments anyone?
>

I think that you're confusing 2 different parts of code here. The
TRIGGER_FIRED_AFTER/TRIGGER_FIRED_BEFORE macros operate on the bits
from the tg_event field of the TriggerData structure. This has a
completely different representation for the trigger timing compared to
the code you mention above, which is from the CreateTrigStmt
structure. That structure is only used internally in a couple of
places, by the parser and CreateTrigger().

I agree that perhaps it would be neater to represent it as an enum,
but I think the scope for code breakage is far smaller than you claim.
This structure is not being exposed to trigger code.

The changes I made in contrib are unrelated to the representation used
in CreateTrigStmt. It's just a matter of tidying up code in before
triggers to say "if (!fired_before) error" rather than "if
(fired_after) error". That's neater anyway, even in the case where
they're mutually exclusive (as they are on tables). The scope for user
code being broken is limited beause they'd have to put one of these
trigger functions in an INSTEAD OF trigger on a view.

Regards,
Dean



>                        regards, tom lane
>