Re: [BUGS] BUG #14808: V10-beta4, backend abort

I wrote:
> What is happening is that the AFTER triggers are queuing more triggers,
> which have TransitionCaptureStates associated, but 
> ExecEndModifyTable thinks it can DestroyTransitionCaptureState
> unconditionally.  When the subsidiary triggers eventually get executed,
> their ats_transition_capture pointers are pointing at garbage.

On closer inspection, the issue is specific to TCS-using AFTER triggers
that are being fired as a result of foreign key enforcement triggers.
In the example, each row deleted from myTbl1 causes firing of the
ON DELETE CASCADE enforcement trigger, which will execute a DELETE
against myTbl3.  That won't delete anything (since myTbl3 is empty)
but we nonetheless queue a firing of the TCS-using AFTER STATEMENT
trigger for myTbl3.

Now, the ExecEndModifyTable instance for the DELETE supposes that
all TCS-using triggers must have been fired during ExecutorFinish;
but *that doesn't happen if EXEC_FLAG_SKIP_TRIGGERS is set*, which
it is because ri_PerformCheck tells SPI not to fire triggers.

I do not recall the exact details of why that is, but I believe
the intention is to fire RI-triggered triggers at the end of the
outer statement rather than exposing the individual RI action as
a statement.

I made a quick hack to not delete the TCS if EXEC_FLAG_SKIP_TRIGGERS
is set, as attached.  The example succeeds given this.  However,
note that the AFTER STATEMENT trigger will be fired once per myTbl1 row
deletion, each time seeing a transition table consisting of only the
myTbl3 rows that went away as a result of that one single row deletion.
I'm not sure that this is per the letter or spirit of the SQL spec.

If it isn't, I don't think there is much we can do about it for v10.
In v11 or later, we could think about somehow merging the transition
tables for all the RI actions triggered by one statement.  This might
well need to go along with rewriting the RI framework to use statement
triggers and TCS state itself.  I think people had already muttered
about doing that.

> The other question that seems pretty relevant is why the subsidiary
> triggers, which are the constraint triggers associated with the
> tables' foreign keys, are getting queued with TransitionCaptureState
> pointers in the first place.  This seems horridly expensive and
> unnecessary.  It also directly contradicts the claim in
> MakeTransitionCaptureState that constraint triggers cannot have
> transition tables.

The other part of the attached patch tweaks AfterTriggerSaveEvent
to not store an ats_transition_capture pointer for a deferrable
trigger event.  This doesn't have anything directly to do with
the current bug, because although the RI triggers are being stored
with such pointers, they aren't actually dereferencing them.  However,
it seems like a good idea anyway, to (1) ensure that we don't have
dangling pointers in the trigger queue, and (2) possibly allow
more merging of shared trigger states.

            regards, tom lane

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index bd84778..49586a3 100644
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
*************** ExecEndModifyTable(ModifyTableState *nod
*** 2318,2325 ****
  {
      int            i;

!     /* Free transition tables */
!     if (node->mt_transition_capture != NULL)
          DestroyTransitionCaptureState(node->mt_transition_capture);

      /*
--- 2318,2331 ----
  {
      int            i;

!     /*
!      * Free transition tables, unless this query is being run in
!      * EXEC_FLAG_SKIP_TRIGGERS mode, which means that it may have queued AFTER
!      * triggers that won't be run till later.  In that case we'll just leak
!      * the transition tables till end of (sub)transaction.
!      */
!     if (node->mt_transition_capture != NULL &&
!         !(node->ps.state->es_top_eflags & EXEC_FLAG_SKIP_TRIGGERS))
          DestroyTransitionCaptureState(node->mt_transition_capture);

      /*
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index da0850b..bbfbc06 100644
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*************** AfterTriggerSaveEvent(EState *estate, Re
*** 5474,5480 ****
          new_shared.ats_tgoid = trigger->tgoid;
          new_shared.ats_relid = RelationGetRelid(rel);
          new_shared.ats_firing_id = 0;
!         new_shared.ats_transition_capture = transition_capture;

          afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth],
                               &new_event, &new_shared);
--- 5474,5482 ----
          new_shared.ats_tgoid = trigger->tgoid;
          new_shared.ats_relid = RelationGetRelid(rel);
          new_shared.ats_firing_id = 0;
!         /* deferrable triggers cannot access transition data */
!         new_shared.ats_transition_capture =
!             trigger->tgdeferrable ? NULL : transition_capture;

          afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth],
                               &new_event, &new_shared);

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