Here is a WIP patch implementing triggers on VIEWs, as outlined in the
proof-of-concept here:
http://archives.postgresql.org/pgsql-hackers/2010-08/msg00160.php

The new triggers allowed on a VIEW are:
1). Statement-level BEFORE INSERT/UPDATE/DELETE
2). Row-level INSTEAD OF INSERT/UPDATE/DELETE
3). Statement-level AFTER INSERT/UPDATE/DELETE

The new INSTEAD OF trigger type may only be used with VIEWs, and may
only be row-level. It does not support the WHEN or FOR UPDATE OF
column_list options.

There are still a number of things left todo:
- extend ALTER VIEW with enable/disable trigger commands
- much more testing
- documentation

and then there's the question of what to do about the concurrency
issues raised by Marko Tiikkaja. Currently it works like Oracle (i.e.,
no locking).

Regards,
Dean

Search Discussions

  • Dean Rasheed at Aug 16, 2010 at 5:31 pm

    On 15 August 2010 18:38, Dean Rasheed wrote:
    There are still a number of things left todo:
    - extend ALTER VIEW with enable/disable trigger commands
    On further reflection, I wonder if the ability to disable VIEW
    triggers is needed/wanted at all. I just noticed that while it is
    possible to disable a RULE on a TABLE, it is not possible to do so on
    VIEW. This certainly makes sense for the _RETURN rule, although
    possibly some people might have a use for disabling other rules on
    views. The situation with triggers is similar - disabling an INSTEAD
    OF trigger would be pointless, and could only lead to errors when
    updating the view. Some people may have a use case for disabling
    BEFORE and AFTER statement triggers on views, but I suspect that the
    number of such cases is small, and I'm tempted to omit this, for now
    at least.

    Thoughts?

    - Dean
  • Tom Lane at Aug 16, 2010 at 5:50 pm

    Dean Rasheed writes:
    On 15 August 2010 18:38, Dean Rasheed wrote:
    There are still a number of things left todo:
    - extend ALTER VIEW with enable/disable trigger commands
    On further reflection, I wonder if the ability to disable VIEW
    triggers is needed/wanted at all.
    AFAIK the only real use for disabling triggers is in connection with
    trigger-based replication systems. A view wouldn't be carrying
    replication-related triggers anyway, so I think this could certainly
    be left out of a first cut.

    You aren't saying that you can see some reason why this couldn't be
    added later, if wanted, correct?

    regards, tom lane
  • Dean Rasheed at Aug 16, 2010 at 6:36 pm

    On 16 August 2010 18:50, Tom Lane wrote:
    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
    On 15 August 2010 18:38, Dean Rasheed wrote:
    There are still a number of things left todo:
    - extend ALTER VIEW with enable/disable trigger commands
    On further reflection, I wonder if the ability to disable VIEW
    triggers is needed/wanted at all.
    AFAIK the only real use for disabling triggers is in connection with
    trigger-based replication systems.  A view wouldn't be carrying
    replication-related triggers anyway, so I think this could certainly
    be left out of a first cut.

    You aren't saying that you can see some reason why this couldn't be
    added later, if wanted, correct?
    Yes. It should be easy to add later if wanted. I just don't see much
    use for it, and I don't want to add more to an already quite big
    patch, if it's not really needed.

    Regards,
    Dean
  • Dean Rasheed at Sep 5, 2010 at 8:10 am

    On 15 August 2010 18:38, Dean Rasheed wrote:
    Here is a WIP patch implementing triggers on VIEWs ... <snip>

    There are still a number of things left todo:
    - extend ALTER VIEW with enable/disable trigger commands
    - much more testing
    - documentation
    Attached is an updated patch with more tests and docs, and a few minor
    code tidy ups. I think that the INSTEAD OF triggers part of the patch
    is compliant with Feature T213 of the SQL 2008 standard. As discussed,
    I don't plan to add the syntax to allow triggers on views to be
    disabled at this time, but that should be easy to implement, if there
    is a use case for it.

    Comments welcome.

    Regards,
    Dean
  • David Christensen at Sep 7, 2010 at 1:06 am

    On Sep 5, 2010, at 3:09 AM, Dean Rasheed wrote:
    On 15 August 2010 18:38, Dean Rasheed wrote:
    Here is a WIP patch implementing triggers on VIEWs ... <snip>

    There are still a number of things left todo:
    - extend ALTER VIEW with enable/disable trigger commands
    - much more testing
    - documentation
    Attached is an updated patch with more tests and docs, and a few minor

    At least for me, there are some portions of the docs which could use some formatting changes in order to not be confusing grammatically; e.g., this was confusing to me on the first read:

    - trigger name. In the case of before triggers, the
    + trigger name. In the case of before and instead of triggers, the

    I realize this lack of formatting was inherited from the existing docs, but this would make more sense to me if this (and presumably the other related instances of "before" and "after") were set apart with <literal></> or similar. This is already in use in some places in this patch, so seems like the correct markup.

    Regards,

    David
    --
    David Christensen
    End Point Corporation
    david@endpoint.com
  • Dean Rasheed at Sep 7, 2010 at 7:43 am

    On 7 September 2010 02:03, David Christensen wrote:
    On Sep 5, 2010, at 3:09 AM, Dean Rasheed wrote:
    On 15 August 2010 18:38, Dean Rasheed wrote:
    Here is a WIP patch implementing triggers on VIEWs ... <snip>

    There are still a number of things left todo:
    - extend ALTER VIEW with enable/disable trigger commands
    - much more testing
    - documentation
    Attached is an updated patch with more tests and docs, and a few minor

    At least for me, there are some portions of the docs which could use some formatting changes in order to not be confusing grammatically; e.g., this was confusing to me on the first read:

    -    trigger name.  In the case of before triggers, the
    +    trigger name.  In the case of before and instead of triggers, the

    I realize this lack of formatting was inherited from the existing docs, but this would make more sense to me if this (and presumably the other related instances of "before" and "after") were set apart with <literal></> or similar.  This is already in use in some places in this patch, so seems like the correct markup.
    Yeah, I think you're right. That chapter is the first place in the
    manual where the concepts of before/after/instead of are introduced.
    The first time it mentions them it uses <firstterm>, but then it
    doesn't use any markup for them for the remainder of the chapter. It
    looks like the rest of the manual uses <literal>+uppercase wherever
    they're mentioned, which does help readability, so it would make sense
    to bring the rest of that chapter into line with that convention.

    Thanks for looking at this.

    Cheers,
    Dean
  • Dean Rasheed at Sep 8, 2010 at 8:00 am

    On 7 September 2010 08:43, Dean Rasheed wrote:
    On 7 September 2010 02:03, David Christensen wrote:
    On Sep 5, 2010, at 3:09 AM, Dean Rasheed wrote:
    On 15 August 2010 18:38, Dean Rasheed wrote:
    Here is a WIP patch implementing triggers on VIEWs ... <snip>

    There are still a number of things left todo:
    - extend ALTER VIEW with enable/disable trigger commands
    - much more testing
    - documentation
    Attached is an updated patch with more tests and docs, and a few minor

    At least for me, there are some portions of the docs which could use some formatting changes in order to not be confusing grammatically; e.g., this was confusing to me on the first read:

    -    trigger name.  In the case of before triggers, the
    +    trigger name.  In the case of before and instead of triggers, the

    I realize this lack of formatting was inherited from the existing docs, but this would make more sense to me if this (and presumably the other related instances of "before" and "after") were set apart with <literal></> or similar.  This is already in use in some places in this patch, so seems like the correct markup.
    Yeah, I think you're right. That chapter is the first place in the
    manual where the concepts of before/after/instead of are introduced.
    The first time it mentions them it uses <firstterm>, but then it
    doesn't use any markup for them for the remainder of the chapter. It
    looks like the rest of the manual uses <literal>+uppercase wherever
    they're mentioned, which does help readability, so it would make sense
    to bring the rest of that chapter into line with that convention.

    Thanks for looking at this.

    Cheers,
    Dean
    Here's an updated version with improved formatting and a few minor
    wording changes to the triggers chapter.

    Regards,
    Dean
  • Bernd Helmle at Sep 29, 2010 at 7:40 pm
    --On 8. September 2010 09:00:33 +0100 Dean Rasheed
    wrote:
    Here's an updated version with improved formatting and a few minor
    wording changes to the triggers chapter.
    This version doesn't apply clean anymore due to some rejects in
    plainmain.c. Corrected version attached.

    --
    Thanks

    Bernd
  • Dean Rasheed at Sep 30, 2010 at 6:38 am

    On 29 September 2010 20:18, Bernd Helmle wrote:

    --On 8. September 2010 09:00:33 +0100 Dean Rasheed
    wrote:
    Here's an updated version with improved formatting and a few minor
    wording changes to the triggers chapter.
    This version doesn't apply clean anymore due to some rejects in plainmain.c.
    Corrected version attached.
    Ah yes, those pesky bits have been rotting while I wasn't looking.
    Thanks for fixing them!

    Regards,
    Dean
  • Tom Lane at Oct 10, 2010 at 6:08 pm

    Bernd Helmle writes:
    --On 8. September 2010 09:00:33 +0100 Dean Rasheed
    wrote:
    Here's an updated version with improved formatting and a few minor
    wording changes to the triggers chapter.
    This version doesn't apply clean anymore due to some rejects in
    plainmain.c. Corrected version attached.
    Applied with revisions. A couple of points worth remarking:

    * I took out this change in planmain.c:

    + /*
    + * If the query target is a VIEW, it won't be in the jointree, but we
    + * need a dummy RelOptInfo node for it. This need not have any stats in
    + * it because it always just goes at the top of the plan tree.
    + */
    + if (parse->resultRelation &&
    + root->simple_rel_array[parse->resultRelation] == NULL)
    + build_simple_rel(root, parse->resultRelation, RELOPT_OTHER_MEMBER_REL);

    AFAICT that's just dead code: the only reason to build such a rel would
    be if there were Vars referencing it in the main part of the plan tree.
    But there aren't. Perhaps this was left over from some early iteration
    of the patch before you had the Var numbering done right? Do you know
    of any cases where it's still needed?

    * I also took out the changes in preprocess_targetlist() that tried to
    prevent equivalent wholerow vars from getting entered in the targetlist.
    This would not work as intended since the executor has specific
    expectations for the names of those junk TLEs; it'd fail if it ever
    actually tried to do an EvalPlanQual recheck that needed those TLEs.
    Now I believe that an EPQ recheck is impossible so far as the update or
    delete itself is concerned, when the target is a view. So if you were
    really concerned about the extra vars, the non-kluge route to a solution
    would be to avoid generating RowMarks in the first place. You'd have to
    think a bit about the possibility of SELECT FOR UPDATE in sub-selects
    though; the query as a whole might need some rowmarks even if the top
    level Modify node doesn't. On the whole I couldn't get excited about
    this issue, so I just left it alone.

    regards, tom lane
  • Dean Rasheed at Oct 11, 2010 at 7:54 am

    On 10 October 2010 19:06, Tom Lane wrote:
    Applied with revisions.
    Brilliant! Thank you very much.
    * I took out this change in planmain.c:

    +       /*
    +        * If the query target is a VIEW, it won't be in the jointree, but we
    +        * need a dummy RelOptInfo node for it. This need not have any stats in
    +        * it because it always just goes at the top of the plan tree.
    +        */
    +       if (parse->resultRelation &&
    +               root->simple_rel_array[parse->resultRelation] == NULL)
    +               build_simple_rel(root, parse->resultRelation, RELOPT_OTHER_MEMBER_REL);

    AFAICT that's just dead code: the only reason to build such a rel would
    be if there were Vars referencing it in the main part of the plan tree.
    But there aren't.  Perhaps this was left over from some early iteration
    of the patch before you had the Var numbering done right?  Do you know
    of any cases where it's still needed?
    No, I think you're right. It was just the leftovers of an early
    attempt to get the rewriter changes right.
    * I also took out the changes in preprocess_targetlist() that tried to
    prevent equivalent wholerow vars from getting entered in the targetlist.
    This would not work as intended since the executor has specific
    expectations for the names of those junk TLEs; it'd fail if it ever
    actually tried to do an EvalPlanQual recheck that needed those TLEs.
    Ah yes, I missed that. I still don't see where it uses those TLEs by
    name though. It looks as though it's using wholeAttNo, so perhaps my
    code would have worked if I had set wholeAttNo on the RowMark? Anyway,
    I don't think it's likely that this extra Var is going to be present
    often in practice, so I don't think it's a problem worth worrying
    about.

    Thanks very much for looking at this.

    Regards,
    Dean

    Now I believe that an EPQ recheck is impossible so far as the update or
    delete itself is concerned, when the target is a view.  So if you were
    really concerned about the extra vars, the non-kluge route to a solution
    would be to avoid generating RowMarks in the first place.  You'd have to
    think a bit about the possibility of SELECT FOR UPDATE in sub-selects
    though; the query as a whole might need some rowmarks even if the top
    level Modify node doesn't.  On the whole I couldn't get excited about
    this issue, so I just left it alone.

    regards, tom lane
  • Bernd Helmle at Sep 22, 2010 at 10:33 pm
    --On 5. September 2010 09:09:55 +0100 Dean Rasheed
    wrote:

    I had a first look on your patch, great work!
    Attached is an updated patch with more tests and docs, and a few minor
    code tidy ups. I think that the INSTEAD OF triggers part of the patch
    is compliant with Feature T213 of the SQL 2008 standard. As discussed,
    Reading the past discussions, there was some mention about the RETURNING
    clause.
    I see Oracle doesn't allow its RETURNING INTO clause with INSTEAD OF
    triggers (at least my 10g XE instance here doesn't allow it, not sure about
    newer versions). I assume the following example might have some surprising
    effects on users:

    CREATE TABLE foo(id integer);
    CREATE VIEW vfoo AS SELECT 'bernd', * FROM foo;

    CREATE OR REPLACE FUNCTION insert_foo() RETURNS trigger
    LANGUAGE plpgsql
    AS $$
    BEGIN INSERT INTO foo VALUES(NEW.id);
    RETURN NEW;
    END; $$;

    CREATE TRIGGER insert_vfoo INSTEAD OF INSERT ON vfoo
    FOR EACH ROW EXECUTE PROCEDURE insert_foo();

    INSERT INTO vfoo VALUES('helmle', 2) RETURNING *;
    text | id
    --------+----
    helmle | 2
    (1 row)

    SELECT * FROM vfoo;
    text | id
    -------+----
    bernd | 2
    (1 row)

    This is solvable by a properly designed trigger function, but maybe we need
    to do something about this?
    I don't plan to add the syntax to allow triggers on views to be
    disabled at this time, but that should be easy to implement, if there
    is a use case for it.
    I really don't see a need for this at the moment. We don't have DISABLE
    RULE either. I'm going to post some additional comments once i've
    understand all things.

    --
    Thanks

    Bernd
  • Marko Tiikkaja at Sep 22, 2010 at 11:27 pm

    On 2010-09-23 1:16 AM, Bernd Helmle wrote:
    INSERT INTO vfoo VALUES('helmle', 2) RETURNING *;
    text | id
    --------+----
    helmle | 2
    (1 row)

    SELECT * FROM vfoo;
    text | id
    -------+----
    bernd | 2
    (1 row)

    This is solvable by a properly designed trigger function, but maybe we need
    to do something about this?
    I really don't think we should limit what people are allowed to do in
    the trigger function.

    Besides, even if the RETURNING clause returned 'bernd' in the above
    case, I think it would be even *more* surprising. The trigger function
    explicitly returns NEW which has 'helmle' as the first field.


    Regards,
    Marko Tiikkaja
  • Dean Rasheed at Sep 23, 2010 at 7:59 am

    On 23 September 2010 00:26, Marko Tiikkaja wrote:
    On 2010-09-23 1:16 AM, Bernd Helmle wrote:

    INSERT INTO vfoo VALUES('helmle', 2) RETURNING *;
    text  | id
    --------+----
    helmle |  2
    (1 row)

    SELECT * FROM vfoo;
    text  | id
    -------+----
    bernd |  2
    (1 row)

    This is solvable by a properly designed trigger function, but maybe we
    need
    to do something about this?
    I really don't think we should limit what people are allowed to do in the
    trigger function.

    Besides, even if the RETURNING clause returned 'bernd' in the above case, I
    think it would be even *more* surprising.  The trigger function explicitly
    returns NEW which has 'helmle' as the first field.
    Yes, I agree. To me this is the least surprising behaviour. I think a
    more common case would be where the trigger computed a value (such as
    the 'last updated' example). The executor doesn't have any kind of a
    handle on the row inserted by the trigger, so it has to rely on the
    function return value to support RETURNING.

    I can confirm the latest Oracle (11g R2 Enterprise Edition) does not
    support RETURNING INTO with INSTEAD OF triggers (although it does work
    with its auto-updatable views), presumably because it's triggers don't
    return values, but I think it would be a shame for us to not support
    it.

    Regards,
    Dean
  • Bernd Helmle at Sep 23, 2010 at 12:05 pm
    --On 23. September 2010 08:59:32 +0100 Dean Rasheed
    wrote:
    Yes, I agree. To me this is the least surprising behaviour. I think a
    more common case would be where the trigger computed a value (such as
    the 'last updated' example). The executor doesn't have any kind of a
    handle on the row inserted by the trigger, so it has to rely on the
    function return value to support RETURNING.
    I didn't mean to forbid it altogether, but at least to document
    explicitely, that the trigger returns a VIEW's NEW tuple, not the one of
    the base table (and may modify it). But you've already adressed this in
    your doc patches, so nothing to worry about further.

    --
    Thanks

    Bernd
  • Bernd Helmle at Oct 5, 2010 at 8:26 pm
    --On 30. September 2010 07:38:18 +0100 Dean Rasheed
    wrote:
    This version doesn't apply clean anymore due to some rejects in
    plainmain.c. Corrected version attached.
    Ah yes, those pesky bits have been rotting while I wasn't looking.
    Thanks for fixing them!
    Basic summary of this patch:

    * 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.

    * 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?

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

    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

    Bernd
  • Dean Rasheed at Oct 6, 2010 at 8:20 am

    On 5 October 2010 21:17, Bernd Helmle 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
  • Tom Lane at Oct 8, 2010 at 3:50 pm

    Bernd Helmle 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
  • Dean Rasheed at Oct 8, 2010 at 4:49 pm

    On 8 October 2010 16:50, Tom Lane 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
  • Tom Lane at Oct 8, 2010 at 5:03 pm

    Dean Rasheed writes:
    On 8 October 2010 16:50, Tom Lane wrote:
    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.
    Oh, they're not allowed on tables? Why not? AFAICS they'd be exactly
    equivalent to a BEFORE trigger that always returns NULL.
    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.
    Yeah, I'm aware of that. My point is that all code that deals with
    trigger firing times now has to consider three possible states where
    before there were two; and it's entirely likely that some places are
    testing for the wrong condition once a third state is admitted as a
    possibility.
    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.
    Well, the only reason those tests are being made at all is as
    self-defense against being called via an incorrect trigger definition.
    So they're worth nothing if they fail to treat the INSTEAD OF case
    correctly.

    regards, tom lane
  • Tom Lane at Oct 8, 2010 at 7:49 pm
    BTW, while I'm looking at this: it seems like the "index" arrays in
    struct TrigDesc are really a lot more complication than they are worth.
    It'd be far easier to dispense with them and instead iterate through
    the main trigger array, skipping any triggers whose tgtype doesn't match
    what we need. If you had a really huge number of triggers on a table,
    it's possible that could be marginally slower, but I'm having a hard
    time envisioning practical situations where anybody could tell the
    difference.

    regards, tom lane
  • Robert Haas at Oct 8, 2010 at 5:44 pm

    On Fri, Oct 8, 2010 at 11:50 AM, Tom Lane wrote:
    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.
    +1. That seems much nicer.
    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'm less sold on this one.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Tom Lane at Oct 8, 2010 at 5:59 pm

    Robert Haas writes:
    On Fri, Oct 8, 2010 at 11:50 AM, Tom Lane wrote:
    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'm less sold on this one.
    I'm not sold on it either, just wanted to run it up the flagpole to see
    if anyone would salute. For the moment I'm thinking that calling out
    the point in the 9.1 release notes should be sufficient. I made an
    extra commit to make sure the issue is salient in the commit log.

    regards, tom lane

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedAug 15, '10 at 5:38p
activeOct 11, '10 at 7:54a
posts24
users6
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase