Re: BUG #14808: V10-beta4, backend abort

On Mon, Sep 11, 2017 at 7:02 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> 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.
>
> In view of the fact that 10rc1 wraps tomorrow, there doesn't seem
> to be time to do anything better about this than my quick hack.
> So I went ahead and pushed that, with a regression test case.

Thank you for the testing and report Philippe, and for the analysis and fix Tom.

@@ -2318,8 +2318,14 @@ ExecEndModifyTable(ModifyTableState *node)
{
int i;

- /* Free transition tables */
- if (node->mt_transition_capture != NULL)
+ /*
+ * 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);

As an idea for a v11 patch, I wonder if it would be better to count
references instead of leaking the TCS as soon as fk-on-cascade
triggers enter the picture. The ModifyTable node would release its
reference in ExecEndModifyTable(), and the queued events' references
would be counted too. I briefly considered a scheme like that before
proposing 501ed02c, but concluded, as it turns out incorrectly, that
there was no way for a trigger referencing node->mt_transition_capture
to fire after that point.

--
Thomas Munro
http://www.enterprisedb.com