[HACKERS] Sync BEFORE STATEMENT trigger behavior with AFTER STATEMENT - Mailing list pgsql-hackers

The just-committed patch 0f79440fb arranges to suppress extra firings
of AFTER STATEMENT triggers that historically have occurred when
several FK enforcement trigger firings affect the same target table.
This leaves us in a situation where you may get more BEFORE STATEMENT
trigger firings than AFTER STATEMENT firings, which seems weird.

I did the attached simple patch to change this, using state very
similar to that now used for AFTER STATEMENT triggers.  It works, in
terms of syncing the number of trigger calls.  The timing of the
calls might seem a bit weird, as shown in the modified regression
test case.  In the corner cases where you can get multiple statement
trigger firings out of one original SQL statement, the ordering
tends to look like
    BEFORE STATEMENT
    BEFORE STATEMENT
    AFTER STATEMENT
    AFTER STATEMENT
not what you might expect,
    BEFORE STATEMENT
    AFTER STATEMENT
    BEFORE STATEMENT
    AFTER STATEMENT
I'm not sure there's anything we can or should do about that, though.
Certainly we don't want to delay the execution of BEFORE calls.

I think we should push this into v10 so that we maintain consistency
of BEFORE vs. AFTER STATEMENT behavior.  Objections?

            regards, tom lane

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 7e391a1..78e6ce7 100644
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*************** static void AfterTriggerSaveEvent(EState
*** 100,105 ****
--- 100,106 ----
                        List *recheckIndexes, Bitmapset *modifiedCols,
                        TransitionCaptureState *transition_capture);
  static void AfterTriggerEnlargeQueryState(void);
+ static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType);


  /*
*************** ExecBSInsertTriggers(EState *estate, Res
*** 2229,2234 ****
--- 2230,2240 ----
      if (!trigdesc->trig_insert_before_statement)
          return;

+     /* no-op if we already fired BS triggers in this context */
+     if (before_stmt_triggers_fired(RelationGetRelid(relinfo->ri_RelationDesc),
+                                    CMD_INSERT))
+         return;
+
      LocTriggerData.type = T_TriggerData;
      LocTriggerData.tg_event = TRIGGER_EVENT_INSERT |
          TRIGGER_EVENT_BEFORE;
*************** ExecBSDeleteTriggers(EState *estate, Res
*** 2439,2444 ****
--- 2445,2455 ----
      if (!trigdesc->trig_delete_before_statement)
          return;

+     /* no-op if we already fired BS triggers in this context */
+     if (before_stmt_triggers_fired(RelationGetRelid(relinfo->ri_RelationDesc),
+                                    CMD_DELETE))
+         return;
+
      LocTriggerData.type = T_TriggerData;
      LocTriggerData.tg_event = TRIGGER_EVENT_DELETE |
          TRIGGER_EVENT_BEFORE;
*************** ExecBSUpdateTriggers(EState *estate, Res
*** 2651,2656 ****
--- 2662,2672 ----
      if (!trigdesc->trig_update_before_statement)
          return;

+     /* no-op if we already fired BS triggers in this context */
+     if (before_stmt_triggers_fired(RelationGetRelid(relinfo->ri_RelationDesc),
+                                    CMD_UPDATE))
+         return;
+
      updatedCols = GetUpdatedColumns(relinfo, estate);

      LocTriggerData.type = T_TriggerData;
*************** struct AfterTriggersTableData
*** 3576,3583 ****
      Oid            relid;            /* target table's OID */
      CmdType        cmdType;        /* event type, CMD_INSERT/UPDATE/DELETE */
      bool        closed;            /* true when no longer OK to add tuples */
!     bool        stmt_trig_done; /* did we already queue stmt-level triggers? */
!     AfterTriggerEventList stmt_trig_events; /* if so, saved list pointer */
      Tuplestorestate *old_tuplestore;    /* "old" transition table, if any */
      Tuplestorestate *new_tuplestore;    /* "new" transition table, if any */
  };
--- 3592,3600 ----
      Oid            relid;            /* target table's OID */
      CmdType        cmdType;        /* event type, CMD_INSERT/UPDATE/DELETE */
      bool        closed;            /* true when no longer OK to add tuples */
!     bool        before_trig_done;    /* did we already queue BS triggers? */
!     bool        after_trig_done;    /* did we already queue AS triggers? */
!     AfterTriggerEventList after_trig_events;    /* if so, saved list pointer */
      Tuplestorestate *old_tuplestore;    /* "old" transition table, if any */
      Tuplestorestate *new_tuplestore;    /* "new" transition table, if any */
  };
*************** AfterTriggerSaveEvent(EState *estate, Re
*** 5651,5656 ****
--- 5668,5704 ----
  }

  /*
+  * Detect whether we already queued BEFORE STATEMENT triggers for the given
+  * relation + operation, and set the flag so the next call will report "true".
+  */
+ static bool
+ before_stmt_triggers_fired(Oid relid, CmdType cmdType)
+ {
+     bool        result;
+     AfterTriggersTableData *table;
+
+     /* Check state, like AfterTriggerSaveEvent. */
+     if (afterTriggers.query_depth < 0)
+         elog(ERROR, "before_stmt_triggers_fired() called outside of query");
+
+     /* Be sure we have enough space to record events at this query depth. */
+     if (afterTriggers.query_depth >= afterTriggers.maxquerydepth)
+         AfterTriggerEnlargeQueryState();
+
+     /*
+      * We keep this state in the AfterTriggersTableData that also holds
+      * transition tables for the relation + operation.  In this way, if we are
+      * forced to make a new set of transition tables because more tuples get
+      * entered after we've already fired triggers, we will allow a new set of
+      * statement triggers to get queued.
+      */
+     table = GetAfterTriggersTableData(relid, cmdType);
+     result = table->before_trig_done;
+     table->before_trig_done = true;
+     return result;
+ }
+
+ /*
   * If we previously queued a set of AFTER STATEMENT triggers for the given
   * relation + operation, and they've not been fired yet, cancel them.  The
   * caller will queue a fresh set that's after any row-level triggers that may
*************** cancel_prior_stmt_triggers(Oid relid, Cm
*** 5684,5690 ****
       */
      table = GetAfterTriggersTableData(relid, cmdType);

!     if (table->stmt_trig_done)
      {
          /*
           * We want to start scanning from the tail location that existed just
--- 5732,5738 ----
       */
      table = GetAfterTriggersTableData(relid, cmdType);

!     if (table->after_trig_done)
      {
          /*
           * We want to start scanning from the tail location that existed just
*************** cancel_prior_stmt_triggers(Oid relid, Cm
*** 5695,5704 ****
          AfterTriggerEvent event;
          AfterTriggerEventChunk *chunk;

!         if (table->stmt_trig_events.tail)
          {
!             chunk = table->stmt_trig_events.tail;
!             event = (AfterTriggerEvent) table->stmt_trig_events.tailfree;
          }
          else
          {
--- 5743,5752 ----
          AfterTriggerEvent event;
          AfterTriggerEventChunk *chunk;

!         if (table->after_trig_events.tail)
          {
!             chunk = table->after_trig_events.tail;
!             event = (AfterTriggerEvent) table->after_trig_events.tailfree;
          }
          else
          {
*************** cancel_prior_stmt_triggers(Oid relid, Cm
*** 5737,5744 ****
  done:

      /* In any case, save current insertion point for next time */
!     table->stmt_trig_done = true;
!     table->stmt_trig_events = qs->events;
  }

  /*
--- 5785,5792 ----
  done:

      /* In any case, save current insertion point for next time */
!     table->after_trig_done = true;
!     table->after_trig_events = qs->events;
  }

  /*
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 3ab6be3..0a04325 100644
*** a/src/test/regress/expected/triggers.out
--- b/src/test/regress/expected/triggers.out
*************** drop table refd_table, trig_table;
*** 2338,2343 ****
--- 2338,2346 ----
  --
  create table self_ref (a int primary key,
                         b int references self_ref(a) on delete cascade);
+ create trigger self_ref_before_del_trig
+   before delete on self_ref
+   for each statement execute procedure trigger_func('self_ref');
  create trigger self_ref_r_trig
    after delete on self_ref referencing old table as old_table
    for each row execute procedure dump_delete();
*************** create trigger self_ref_s_trig
*** 2346,2352 ****
--- 2349,2357 ----
    for each statement execute procedure dump_delete();
  insert into self_ref values (1, null), (2, 1), (3, 2);
  delete from self_ref where a = 1;
+ NOTICE:  trigger_func(self_ref) called: action = DELETE, when = BEFORE, level = STATEMENT
  NOTICE:  trigger = self_ref_r_trig, old table = (1,), (2,1)
+ NOTICE:  trigger_func(self_ref) called: action = DELETE, when = BEFORE, level = STATEMENT
  NOTICE:  trigger = self_ref_r_trig, old table = (1,), (2,1)
  NOTICE:  trigger = self_ref_s_trig, old table = (1,), (2,1)
  NOTICE:  trigger = self_ref_r_trig, old table = (3,2)
*************** NOTICE:  trigger = self_ref_s_trig, old
*** 2355,2360 ****
--- 2360,2366 ----
  drop trigger self_ref_r_trig on self_ref;
  insert into self_ref values (1, null), (2, 1), (3, 2), (4, 3);
  delete from self_ref where a = 1;
+ NOTICE:  trigger_func(self_ref) called: action = DELETE, when = BEFORE, level = STATEMENT
  NOTICE:  trigger = self_ref_s_trig, old table = (1,), (2,1), (3,2), (4,3)
  drop table self_ref;
  -- cleanup
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 30bb7d1..2aa3120 100644
*** a/src/test/regress/sql/triggers.sql
--- b/src/test/regress/sql/triggers.sql
*************** drop table refd_table, trig_table;
*** 1834,1839 ****
--- 1834,1842 ----
  create table self_ref (a int primary key,
                         b int references self_ref(a) on delete cascade);

+ create trigger self_ref_before_del_trig
+   before delete on self_ref
+   for each statement execute procedure trigger_func('self_ref');
  create trigger self_ref_r_trig
    after delete on self_ref referencing old table as old_table
    for each row execute procedure dump_delete();

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers