Fix deferred FK check batching introduced by commit b7b27eb41a5

That commit introduced AfterTriggerIsActive() to detect whether
we are inside the after-trigger firing machinery, so that RI trigger
functions can take the batched fast path.  It was implemented using
query_depth >= 0, which correctly identified immediate trigger firing
but missed the deferred case where query_depth is -1 at COMMIT via
AfterTriggerFireDeferred().  This caused deferred FK checks to fall
back to the per-row fast path instead of the batched path.

The correct check is whether we are inside an after-trigger firing
loop specifically.  Introduce afterTriggerFiringDepth, a counter
incremented around the trigger-firing loops in AfterTriggerEndQuery,
AfterTriggerFireDeferred, and AfterTriggerSetState, and decremented
after FireAfterTriggerBatchCallbacks() returns.  AfterTriggerIsActive()
now returns afterTriggerFiringDepth > 0.

Reported-by: Chao Li <li.evan.chao@gmail.com>
Author: Chao Li <li.evan.chao@gmail.com>
Co-authored-by: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/C2133B47-79CD-40FF-B088-02D20D654806@gmail.com

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 4d4e96a53023696c8cc5591dd570920b84c8eeb2..c41005ba44e6294e7ebe182bb67e009b86cd796f 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3940,6 +3940,13 @@ typedef struct AfterTriggerCallbackItem
 
 static AfterTriggersData afterTriggers;
 
+/*
+ * Incremented before invoking afterTriggerInvokeEvents().  Used by
+ * AfterTriggerIsActive() to determine whether batch callbacks will fire,
+ * so that RI trigger functions can take the batched fast path.
+ */
+static int     afterTriggerFiringDepth = 0;
+
 static void AfterTriggerExecute(EState *estate,
                                                                AfterTriggerEvent event,
                                                                ResultRelInfo *relInfo,
@@ -5113,6 +5120,7 @@ AfterTriggerBeginXact(void)
        Assert(afterTriggers.events.head == NULL);
        Assert(afterTriggers.trans_stack == NULL);
        Assert(afterTriggers.maxtransdepth == 0);
+       Assert(afterTriggerFiringDepth == 0);
 }
 
 
@@ -5184,6 +5192,7 @@ AfterTriggerEndQuery(EState *estate)
         */
        qs = &afterTriggers.query_stack[afterTriggers.query_depth];
 
+       afterTriggerFiringDepth++;
        for (;;)
        {
                if (afterTriggerMarkEvents(&qs->events, &afterTriggers.events, true))
@@ -5234,6 +5243,7 @@ AfterTriggerEndQuery(EState *estate)
        AfterTriggerFreeQuery(&afterTriggers.query_stack[afterTriggers.query_depth]);
 
        afterTriggers.query_depth--;
+       afterTriggerFiringDepth--;
 }
 
 
@@ -5329,6 +5339,7 @@ AfterTriggerFireDeferred(void)
         * Run all the remaining triggers.  Loop until they are all gone, in case
         * some trigger queues more for us to do.
         */
+       afterTriggerFiringDepth++;
        while (afterTriggerMarkEvents(events, NULL, false))
        {
                CommandId       firing_id = afterTriggers.firing_counter++;
@@ -5340,6 +5351,8 @@ AfterTriggerFireDeferred(void)
        /* Flush any fast-path batches accumulated by the triggers just fired. */
        FireAfterTriggerBatchCallbacks();
 
+       afterTriggerFiringDepth--;
+
        /*
         * We don't bother freeing the event list, since it will go away anyway
         * (and more efficiently than via pfree) in AfterTriggerEndXact.
@@ -5404,6 +5417,8 @@ AfterTriggerEndXact(bool isCommit)
 
        /* No more afterTriggers manipulation until next transaction starts. */
        afterTriggers.query_depth = -1;
+
+       afterTriggerFiringDepth = 0;
 }
 
 /*
@@ -6053,6 +6068,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
                AfterTriggerEventList *events = &afterTriggers.events;
                bool            snapshot_set = false;
 
+               afterTriggerFiringDepth++;
                while (afterTriggerMarkEvents(events, NULL, true))
                {
                        CommandId       firing_id = afterTriggers.firing_counter++;
@@ -6086,6 +6102,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
                 * Flush any fast-path batches accumulated by the triggers just fired.
                 */
                FireAfterTriggerBatchCallbacks();
+               afterTriggerFiringDepth--;
 
                if (snapshot_set)
                        PopActiveSnapshot();
@@ -6806,10 +6823,10 @@ RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback,
         * Allocate in TopTransactionContext so the item survives for the duration
         * of the batch, which may span multiple trigger invocations.
         *
-        * Must be called while afterTriggers is active (query_depth >= 0);
-        * callbacks registered outside a trigger-firing context would never fire.
+        * Must be called while afterTriggers is active; callbacks registered
+        * outside a trigger-firing context would never fire.
         */
-       Assert(afterTriggers.query_depth >= 0);
+       Assert(afterTriggerFiringDepth > 0);
        oldcxt = MemoryContextSwitchTo(TopTransactionContext);
        item = palloc(sizeof(AfterTriggerCallbackItem));
        item->callback = callback;
@@ -6836,6 +6853,7 @@ FireAfterTriggerBatchCallbacks(void)
        if (afterTriggers.query_depth > 0)
                return;
 
+       Assert(afterTriggerFiringDepth > 0);
        foreach(lc, afterTriggers.batch_callbacks)
        {
                AfterTriggerCallbackItem *item = lfirst(lc);
@@ -6858,5 +6876,5 @@ FireAfterTriggerBatchCallbacks(void)
 bool
 AfterTriggerIsActive(void)
 {
-       return afterTriggers.query_depth >= 0;
+       return afterTriggerFiringDepth > 0;
 }