Thread: Possible patch to remove "triggered data change" support

Possible patch to remove "triggered data change" support

From
Tom Lane
Date:
Attached is a patch to remove the "triggered data change" error check
from trigger.c, as well as pick up the performance improvements that
that allows (due to not having to save deferred trigger events as long).

I am not yet proposing to actually commit this, since the discussion
about whether it's a good idea is still ongoing in -hackers.  But
Stephan seemed to want to have a copy for testing purposes, and I
thought other people might be interested too.

            regards, tom lane

*** src/backend/commands/trigger.c.orig    Sun Nov 11 19:00:55 2001
--- src/backend/commands/trigger.c    Mon Nov 12 18:31:58 2001
***************
*** 935,944 ****
  {
      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;

!     /* Must save info if there are any deferred triggers on this rel */
!     if (trigdesc->n_after_row[TRIGGER_EVENT_INSERT] > 0 ||
!         trigdesc->n_after_row[TRIGGER_EVENT_UPDATE] > 0 ||
!         trigdesc->n_after_row[TRIGGER_EVENT_DELETE] > 0)
          DeferredTriggerSaveEvent(relinfo, TRIGGER_EVENT_INSERT,
                                   NULL, trigtuple);
  }
--- 935,941 ----
  {
      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;

!     if (trigdesc->n_after_row[TRIGGER_EVENT_INSERT] > 0)
          DeferredTriggerSaveEvent(relinfo, TRIGGER_EVENT_INSERT,
                                   NULL, trigtuple);
  }
***************
*** 1000,1008 ****
  {
      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;

!     /* Must save info if there are upd/del deferred triggers on this rel */
!     if (trigdesc->n_after_row[TRIGGER_EVENT_UPDATE] > 0 ||
!         trigdesc->n_after_row[TRIGGER_EVENT_DELETE] > 0)
      {
          HeapTuple    trigtuple = GetTupleForTrigger(estate, relinfo,
                                                     tupleid, NULL);
--- 997,1003 ----
  {
      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;

!     if (trigdesc->n_after_row[TRIGGER_EVENT_DELETE] > 0)
      {
          HeapTuple    trigtuple = GetTupleForTrigger(estate, relinfo,
                                                     tupleid, NULL);
***************
*** 1077,1085 ****
  {
      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;

!     /* Must save info if there are upd/del deferred triggers on this rel */
!     if (trigdesc->n_after_row[TRIGGER_EVENT_UPDATE] > 0 ||
!         trigdesc->n_after_row[TRIGGER_EVENT_DELETE] > 0)
      {
          HeapTuple    trigtuple = GetTupleForTrigger(estate, relinfo,
                                                     tupleid, NULL);
--- 1072,1078 ----
  {
      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;

!     if (trigdesc->n_after_row[TRIGGER_EVENT_UPDATE] > 0)
      {
          HeapTuple    trigtuple = GetTupleForTrigger(estate, relinfo,
                                                     tupleid, NULL);
***************
*** 1208,1224 ****
  static List *deftrig_trigstates;

  /* ----------
!  * The list of events during the entire transaction.  deftrig_events
!  * is the head, deftrig_event_tail is the last entry.  Because this can
!  * grow pretty large, we don't use separate List nodes, but instead thread
!  * the list through the dte_next fields of the member nodes.  Saves just a
!  * few bytes per entry, but that adds up.
   *
   * XXX Need to be able to shove this data out to a file if it grows too
   *       large...
   * ----------
   */
- static int    deftrig_n_events;
  static DeferredTriggerEvent deftrig_events;
  static DeferredTriggerEvent deftrig_event_tail;

--- 1201,1217 ----
  static List *deftrig_trigstates;

  /* ----------
!  * The list of pending deferred trigger events during the current transaction.
!  *
!  * deftrig_events is the head, deftrig_event_tail is the last entry.
!  * Because this can grow pretty large, we don't use separate List nodes,
!  * but instead thread the list through the dte_next fields of the member
!  * nodes.  Saves just a few bytes per entry, but that adds up.
   *
   * XXX Need to be able to shove this data out to a file if it grows too
   *       large...
   * ----------
   */
  static DeferredTriggerEvent deftrig_events;
  static DeferredTriggerEvent deftrig_event_tail;

***************
*** 1284,1290 ****
   * deferredTriggerAddEvent()
   *
   *    Add a new trigger event to the queue.
-  *    Caller must have switched into appropriate memory context!
   * ----------
   */
  static void
--- 1277,1282 ----
***************
*** 1307,1350 ****
          deftrig_event_tail->dte_next = event;
          deftrig_event_tail = event;
      }
-     deftrig_n_events++;
- }
-
-
- /* ----------
-  * deferredTriggerGetPreviousEvent()
-  *
-  *    Scan the eventlist to find the event a given OLD tuple
-  *    resulted from in the same transaction.
-  * ----------
-  */
- static DeferredTriggerEvent
- deferredTriggerGetPreviousEvent(Oid relid, ItemPointer ctid)
- {
-     DeferredTriggerEvent previous = NULL;
-     DeferredTriggerEvent prev;
-
-     /* Search the list to find the last event affecting this tuple */
-     for (prev = deftrig_events; prev != NULL; prev = prev->dte_next)
-     {
-         if (prev->dte_relid != relid)
-             continue;
-         if (prev->dte_event & TRIGGER_DEFERRED_CANCELED)
-             continue;
-
-         if (ItemPointerGetBlockNumber(ctid) ==
-             ItemPointerGetBlockNumber(&(prev->dte_newctid)) &&
-             ItemPointerGetOffsetNumber(ctid) ==
-             ItemPointerGetOffsetNumber(&(prev->dte_newctid)))
-             previous = prev;
-     }
-
-     if (previous == NULL)
-         elog(ERROR,
-          "deferredTriggerGetPreviousEvent: event for tuple %s not found",
-              DatumGetCString(DirectFunctionCall1(tidout,
-                                                  PointerGetDatum(ctid))));
-     return previous;
  }


--- 1299,1304 ----
***************
*** 1473,1490 ****
  static void
  deferredTriggerInvokeEvents(bool immediate_only)
  {
!     DeferredTriggerEvent event;
      MemoryContext per_tuple_context;
      Relation    rel = NULL;
      FmgrInfo   *finfo = NULL;

      /*
!      * For now we process all events - to speedup transaction blocks we
!      * need to remember the actual end of the queue at EndQuery and
!      * process only events that are newer. On state changes we simply
!      * reset the position to the beginning of the queue and process all
!      * events once with the new states when the SET CONSTRAINTS ...
!      * command finishes and calls EndQuery.
       */

      /* Make a per-tuple memory context for trigger function calls */
--- 1427,1451 ----
  static void
  deferredTriggerInvokeEvents(bool immediate_only)
  {
!     DeferredTriggerEvent event,
!                 prev_event = NULL;
      MemoryContext per_tuple_context;
      Relation    rel = NULL;
      FmgrInfo   *finfo = NULL;

      /*
!      * If immediate_only is true, we remove fully-processed events from
!      * the event queue to recycle space.  If immediate_only is false,
!      * we are going to discard the whole event queue on return anyway,
!      * so no need to bother with "retail" pfree's.
!      *
!      * In a scenario with many commands in a transaction and many
!      * deferred-to-end-of-transaction triggers, it could get annoying
!      * to rescan all the deferred triggers at each command end.
!      * To speed this up, we could remember the actual end of the queue at
!      * EndQuery and examine only events that are newer. On state changes
!      * we simply reset the saved position to the beginning of the queue
!      * and process all events once with the new states.
       */

      /* Make a per-tuple memory context for trigger function calls */
***************
*** 1495,1574 ****
                                ALLOCSET_DEFAULT_INITSIZE,
                                ALLOCSET_DEFAULT_MAXSIZE);

!     for (event = deftrig_events; event != NULL; event = event->dte_next)
      {
!         bool        still_deferred_ones;
          int            i;

          /*
!          * Check if event is completely done.
!          */
!         if (event->dte_event & (TRIGGER_DEFERRED_DONE |
!                                 TRIGGER_DEFERRED_CANCELED))
!             continue;
!
!         MemoryContextReset(per_tuple_context);
!
!         /*
!          * Check each trigger item in the event.
           */
!         still_deferred_ones = false;
!         for (i = 0; i < event->dte_n_items; i++)
          {
!             if (event->dte_item[i].dti_state & TRIGGER_DEFERRED_DONE)
!                 continue;
!
!             /*
!              * This trigger item hasn't been called yet. Check if we
!              * should call it now.
!              */
!             if (immediate_only && deferredTriggerCheckState(
!                                             event->dte_item[i].dti_tgoid,
!                                            event->dte_item[i].dti_state))
!             {
!                 still_deferred_ones = true;
!                 continue;
!             }

              /*
!              * So let's fire it... but first, open the correct relation if
!              * this is not the same relation as before.
               */
!             if (rel == NULL || rel->rd_id != event->dte_relid)
              {
!                 if (rel)
!                     heap_close(rel, NoLock);
!                 if (finfo)
!                     pfree(finfo);

                  /*
!                  * We assume that an appropriate lock is still held by the
!                  * executor, so grab no new lock here.
                   */
!                 rel = heap_open(event->dte_relid, NoLock);

                  /*
!                  * Allocate space to cache fmgr lookup info for triggers
!                  * of this relation.
                   */
!                 finfo = (FmgrInfo *)
!                     palloc(rel->trigdesc->numtriggers * sizeof(FmgrInfo));
!                 MemSet(finfo, 0,
!                        rel->trigdesc->numtriggers * sizeof(FmgrInfo));
!             }

!             DeferredTriggerExecute(event, i, rel, finfo, per_tuple_context);

!             event->dte_item[i].dti_state |= TRIGGER_DEFERRED_DONE;
          }

          /*
!          * Remember in the event itself if all trigger items are done.
           */
!         if (!still_deferred_ones)
!             event->dte_event |= TRIGGER_DEFERRED_DONE;
      }

      if (rel)
          heap_close(rel, NoLock);
      if (finfo)
--- 1456,1573 ----
                                ALLOCSET_DEFAULT_INITSIZE,
                                ALLOCSET_DEFAULT_MAXSIZE);

!     event = deftrig_events;
!     while (event != NULL)
      {
!         bool        still_deferred_ones = false;
!         DeferredTriggerEvent next_event;
          int            i;

          /*
!          * Check if event is already completely done.
           */
!         if (! (event->dte_event & (TRIGGER_DEFERRED_DONE |
!                                    TRIGGER_DEFERRED_CANCELED)))
          {
!             MemoryContextReset(per_tuple_context);

              /*
!              * Check each trigger item in the event.
               */
!             for (i = 0; i < event->dte_n_items; i++)
              {
!                 if (event->dte_item[i].dti_state & TRIGGER_DEFERRED_DONE)
!                     continue;

                  /*
!                  * This trigger item hasn't been called yet. Check if we
!                  * should call it now.
                   */
!                 if (immediate_only &&
!                     deferredTriggerCheckState(event->dte_item[i].dti_tgoid,
!                                               event->dte_item[i].dti_state))
!                 {
!                     still_deferred_ones = true;
!                     continue;
!                 }

                  /*
!                  * So let's fire it... but first, open the correct relation
!                  * if this is not the same relation as before.
                   */
!                 if (rel == NULL || rel->rd_id != event->dte_relid)
!                 {
!                     if (rel)
!                         heap_close(rel, NoLock);
!                     if (finfo)
!                         pfree(finfo);

!                     /*
!                      * We assume that an appropriate lock is still held by the
!                      * executor, so grab no new lock here.
!                      */
!                     rel = heap_open(event->dte_relid, NoLock);

!                     /*
!                      * Allocate space to cache fmgr lookup info for triggers
!                      * of this relation.
!                      */
!                     finfo = (FmgrInfo *)
!                         palloc(rel->trigdesc->numtriggers * sizeof(FmgrInfo));
!                     MemSet(finfo, 0,
!                            rel->trigdesc->numtriggers * sizeof(FmgrInfo));
!                 }
!
!                 DeferredTriggerExecute(event, i, rel, finfo,
!                                        per_tuple_context);
!
!                 event->dte_item[i].dti_state |= TRIGGER_DEFERRED_DONE;
!             } /* end loop over items within event */
          }

          /*
!          * If it's now completely done, throw it away.
!          *
!          * NB: it's possible the trigger calls above added more events to the
!          * queue, or that calls we will do later will want to add more,
!          * so we have to be careful about maintaining list validity here.
           */
!         next_event = event->dte_next;
!
!         if (still_deferred_ones)
!         {
!             /* Not done, keep in list */
!             prev_event = event;
!         }
!         else
!         {
!             /* Done */
!             if (immediate_only)
!             {
!                 /* delink it from list and free it */
!                 if (prev_event)
!                     prev_event->dte_next = next_event;
!                 else
!                     deftrig_events = next_event;
!                 pfree(event);
!             }
!             else
!             {
!                 /*
!                  * We will clean up later, but just for paranoia's sake,
!                  * mark the event done.
!                  */
!                 event->dte_event |= TRIGGER_DEFERRED_DONE;
!             }
!         }
!
!         event = next_event;
      }

+     /* Update list tail pointer in case we just deleted tail event */
+     deftrig_event_tail = prev_event;
+
+     /* Release working resources */
      if (rel)
          heap_close(rel, NoLock);
      if (finfo)
***************
*** 1649,1655 ****

      MemoryContextSwitchTo(oldcxt);

-     deftrig_n_events = 0;
      deftrig_events = NULL;
      deftrig_event_tail = NULL;
  }
--- 1648,1653 ----
***************
*** 1986,1994 ****
   *    Called by ExecAR...Triggers() to add the event to the queue.
   *
   *    NOTE: should be called only if we've determined that an event must
!  *    be added to the queue.    We must save *all* events if there is either
!  *    an UPDATE or a DELETE deferred trigger; see uses of
!  *    deferredTriggerGetPreviousEvent.
   * ----------
   */
  static void
--- 1984,1990 ----
   *    Called by ExecAR...Triggers() to add the event to the queue.
   *
   *    NOTE: should be called only if we've determined that an event must
!  *    be added to the queue.
   * ----------
   */
  static void
***************
*** 1999,2005 ****
      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
      MemoryContext oldcxt;
      DeferredTriggerEvent new_event;
-     DeferredTriggerEvent prev_event;
      int            new_size;
      int            i;
      int            ntriggers;
--- 1995,2000 ----
***************
*** 2060,2085 ****
      switch (event & TRIGGER_EVENT_OPMASK)
      {
          case TRIGGER_EVENT_INSERT:
!             new_event->dte_event |= TRIGGER_DEFERRED_ROW_INSERTED;
!             new_event->dte_event |= TRIGGER_DEFERRED_KEY_CHANGED;
              break;

          case TRIGGER_EVENT_UPDATE:
-
              /*
!              * On UPDATE check if the tuple updated has been inserted or a
!              * foreign referenced key value that's changing now has been
!              * updated once before in this transaction.
!              */
!             if (!TransactionIdEquals(oldtup->t_data->t_xmin,
!                                      GetCurrentTransactionId()))
!                 prev_event = NULL;
!             else
!                 prev_event =
!                     deferredTriggerGetPreviousEvent(rel->rd_id, &oldctid);
!
!             /*
!              * Now check if one of the referenced keys is changed.
               */
              for (i = 0; i < ntriggers; i++)
              {
--- 2055,2066 ----
      switch (event & TRIGGER_EVENT_OPMASK)
      {
          case TRIGGER_EVENT_INSERT:
!             /* nothing to do */
              break;

          case TRIGGER_EVENT_UPDATE:
              /*
!              * Check if one of the referenced keys is changed.
               */
              for (i = 0; i < ntriggers; i++)
              {
***************
*** 2120,2228 ****
                  {
                      /*
                       * The key hasn't changed, so no need later to invoke
!                      * the trigger at all. But remember other states from
!                      * the possible earlier event.
                       */
                      new_event->dte_item[i].dti_state |= TRIGGER_DEFERRED_DONE;
-
-                     if (prev_event)
-                     {
-                         if (prev_event->dte_event &
-                             TRIGGER_DEFERRED_ROW_INSERTED)
-                         {
-                             /*
-                              * This is a row inserted during our
-                              * transaction. So any key value is considered
-                              * changed.
-                              */
-                             new_event->dte_event |=
-                                 TRIGGER_DEFERRED_ROW_INSERTED;
-                             new_event->dte_event |=
-                                 TRIGGER_DEFERRED_KEY_CHANGED;
-                             new_event->dte_item[i].dti_state |=
-                                 TRIGGER_DEFERRED_KEY_CHANGED;
-                         }
-                         else
-                         {
-                             /*
-                              * This is a row, previously updated. So if
-                              * this key has been changed before, we still
-                              * remember that it happened.
-                              */
-                             if (prev_event->dte_item[i].dti_state &
-                                 TRIGGER_DEFERRED_KEY_CHANGED)
-                             {
-                                 new_event->dte_item[i].dti_state |=
-                                     TRIGGER_DEFERRED_KEY_CHANGED;
-                                 new_event->dte_event |=
-                                     TRIGGER_DEFERRED_KEY_CHANGED;
-                             }
-                         }
-                     }
-                 }
-                 else
-                 {
-                     /*
-                      * Bomb out if this key has been changed before.
-                      * Otherwise remember that we do so.
-                      */
-                     if (prev_event)
-                     {
-                         if (prev_event->dte_event &
-                             TRIGGER_DEFERRED_ROW_INSERTED)
-                             elog(ERROR, "triggered data change violation "
-                                  "on relation \"%s\"",
-                              DatumGetCString(DirectFunctionCall1(nameout,
-                                 NameGetDatum(&(rel->rd_rel->relname)))));
-
-                         if (prev_event->dte_item[i].dti_state &
-                             TRIGGER_DEFERRED_KEY_CHANGED)
-                             elog(ERROR, "triggered data change violation "
-                                  "on relation \"%s\"",
-                              DatumGetCString(DirectFunctionCall1(nameout,
-                                 NameGetDatum(&(rel->rd_rel->relname)))));
-                     }
-
-                     /*
-                      * This is the first change to this key, so let it
-                      * happen.
-                      */
-                     new_event->dte_item[i].dti_state |=
-                         TRIGGER_DEFERRED_KEY_CHANGED;
-                     new_event->dte_event |= TRIGGER_DEFERRED_KEY_CHANGED;
                  }
              }

              break;

          case TRIGGER_EVENT_DELETE:
!
!             /*
!              * On DELETE check if the tuple deleted has been inserted or a
!              * possibly referenced key value has changed in this
!              * transaction.
!              */
!             if (!TransactionIdEquals(oldtup->t_data->t_xmin,
!                                      GetCurrentTransactionId()))
!                 break;
!
!             /*
!              * Look at the previous event to the same tuple.
!              */
!             prev_event = deferredTriggerGetPreviousEvent(rel->rd_id, &oldctid);
!             if (prev_event->dte_event & TRIGGER_DEFERRED_KEY_CHANGED)
!                 elog(ERROR, "triggered data change violation "
!                      "on relation \"%s\"",
!                      DatumGetCString(DirectFunctionCall1(nameout,
!                                 NameGetDatum(&(rel->rd_rel->relname)))));
!
              break;
      }

      /*
!      * Anything's fine up to here. Add the new event to the queue.
       */
-     oldcxt = MemoryContextSwitchTo(deftrig_cxt);
      deferredTriggerAddEvent(new_event);
-     MemoryContextSwitchTo(oldcxt);
  }
--- 2101,2121 ----
                  {
                      /*
                       * The key hasn't changed, so no need later to invoke
!                      * the trigger at all.
                       */
                      new_event->dte_item[i].dti_state |= TRIGGER_DEFERRED_DONE;
                  }
              }

              break;

          case TRIGGER_EVENT_DELETE:
!             /* nothing to do */
              break;
      }

      /*
!      * Add the new event to the queue.
       */
      deferredTriggerAddEvent(new_event);
  }
*** src/include/commands/trigger.h.orig    Sun Nov 11 19:46:36 2001
--- src/include/commands/trigger.h    Mon Nov 12 18:11:31 2001
***************
*** 50,58 ****
  #define TRIGGER_DEFERRED_DEFERRABLE        0x00000040
  #define TRIGGER_DEFERRED_INITDEFERRED    0x00000080
  #define TRIGGER_DEFERRED_HAS_BEFORE        0x00000100
- #define TRIGGER_DEFERRED_ROW_INSERTED    0x00000200
- #define TRIGGER_DEFERRED_KEY_CHANGED    0x00000400
- #define TRIGGER_DEFERRED_MASK            0x000007F0

  #define TRIGGER_FIRED_BY_INSERT(event)    \
          (((TriggerEvent) (event) & TRIGGER_EVENT_OPMASK) == \
--- 50,55 ----

Re: Possible patch to remove "triggered data change" support

From
Bruce Momjian
Date:
> Attached is a patch to remove the "triggered data change" error check
> from trigger.c, as well as pick up the performance improvements that
> that allows (due to not having to save deferred trigger events as long).
>
> I am not yet proposing to actually commit this, since the discussion
> about whether it's a good idea is still ongoing in -hackers.  But
> Stephan seemed to want to have a copy for testing purposes, and I
> thought other people might be interested too.

Are you saying we never need to save deferred trigger events any longer,
or just in this case?

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Re: Possible patch to remove "triggered data change" support

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Are you saying we never need to save deferred trigger events any longer,
> or just in this case?

The only reason we save 'em beyond the time they are executed is to support
"triggered data change" detection.

            regards, tom lane

Re: Possible patch to remove "triggered data change" support

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I assume we still need a queue for deferred triggers, right?

There's still a queue.  But it saves fewer events for shorter intervals
than it used to.  Example: in the present code, if you have *any*
deferred triggers (for INSERT, UPDATE, or DELETE) on a rel then queue
entries are made for *all* triggerable events (INSERT, UPDATE, or
DELETE) on that rel.  In the proposed patch, if you have only an AFTER
INSERT trigger then no queue entries are made for UPDATE or DELETE
events, etc.  This has a direct bearing on the number of queue entries
made in RI scenarios, since the RI triggers are for subsets of events.

            regards, tom lane

Re: Possible patch to remove "triggered data change" support

From
Bruce Momjian
Date:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Are you saying we never need to save deferred trigger events any longer,
> > or just in this case?
>
> The only reason we save 'em beyond the time they are executed is to support
> "triggered data change" detection.

I assume we still need a queue for deferred triggers, right?  We just
now don't need to keep them after that like we used to, right?

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026