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.
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?
regards, tom lane
