instrumentation: Separate trigger logic from other uses

Introduce TriggerInstrumentation to capture trigger timing and firings
(previously counted in "ntuples"), to aid a future refactoring that
splits out all Instrumentation fields beyond timing and WAL/buffers into
more specific structs.

In passing, drop the "n" argument to InstrAlloc, as all remaining callers need
exactly one Instrumentation struct.  The duplication between InstrAlloc() and
InstrInit(), as well as the conditional initialization of async_mode will be
addressed in a subsequent commit.

Author: Lukas Fittl <lukas@fittl.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/flat/CAP53PkzdBK8VJ1fS4AZ481LgMN8f9mJiC39ZRHqkFUSYq6KWmg@mail.gmail.com

diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index e856cd35a6f0fb2d906d253dcf2bd6df956bd40a..5f5c1ff0da3e980ce4f99ac672e417166bdbc2ae 100644 (file)
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -315,7 +315,7 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
                        MemoryContext oldcxt;
 
                        oldcxt = MemoryContextSwitchTo(queryDesc->estate->es_query_cxt);
-                       queryDesc->totaltime = InstrAlloc(1, INSTRUMENT_ALL, false);
+                       queryDesc->totaltime = InstrAlloc(INSTRUMENT_ALL, false);
                        MemoryContextSwitchTo(oldcxt);
                }
        }

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 5494d41dca16121ee99ef7d0c22f2262458172ce..ddbd5727ddf5e6ec10ea7ac77d09de9a08565c4a 100644 (file)
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1025,7 +1025,7 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
                        MemoryContext oldcxt;
 
                        oldcxt = MemoryContextSwitchTo(queryDesc->estate->es_query_cxt);
-                       queryDesc->totaltime = InstrAlloc(1, INSTRUMENT_ALL, false);
+                       queryDesc->totaltime = InstrAlloc(INSTRUMENT_ALL, false);
                        MemoryContextSwitchTo(oldcxt);
                }
        }

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e4b70166b0e50c79636ca6864f40cc42d73ae223..5b76cbc26fad277a8ec471d082e08e33305329dc 100644 (file)
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1101,18 +1101,18 @@ report_triggers(ResultRelInfo *rInfo, bool show_relname, ExplainState *es)
        for (nt = 0; nt < rInfo->ri_TrigDesc->numtriggers; nt++)
        {
                Trigger    *trig = rInfo->ri_TrigDesc->triggers + nt;
-               Instrumentation *instr = rInfo->ri_TrigInstrument + nt;
+               TriggerInstrumentation *tginstr = rInfo->ri_TrigInstrument + nt;
                char       *relname;
                char       *conname = NULL;
 
-               /* Must clean up instrumentation state */
-               InstrEndLoop(instr);
+               /* Ensure total timing is updated from the internal counter */
+               InstrEndLoop(&tginstr->instr);
 
                /*
                 * We ignore triggers that were never invoked; they likely aren't
                 * relevant to the current query type.
                 */
-               if (instr->ntuples == 0)
+               if (tginstr->firings == 0)
                        continue;
 
                ExplainOpenGroup("Trigger", NULL, true, es);
@@ -1137,11 +1137,12 @@ report_triggers(ResultRelInfo *rInfo, bool show_relname, ExplainState *es)
                        if (show_relname)
                                appendStringInfo(es->str, " on %s", relname);
                        if (es->timing)
-                               appendStringInfo(es->str, ": time=%.3f calls=%.0f\n",
-                                                                INSTR_TIME_GET_MILLISEC(instr->total),
-                                                                instr->ntuples);
+                               appendStringInfo(es->str, ": time=%.3f calls=%" PRId64 "\n",
+                                                                INSTR_TIME_GET_MILLISEC(tginstr->instr.total),
+                                                                tginstr->firings);
                        else
-                               appendStringInfo(es->str, ": calls=%.0f\n", instr->ntuples);
+                               appendStringInfo(es->str, ": calls=%" PRId64 "\n",
+                                                                tginstr->firings);
                }
                else
                {
@@ -1151,9 +1152,9 @@ report_triggers(ResultRelInfo *rInfo, bool show_relname, ExplainState *es)
                        ExplainPropertyText("Relation", relname, es);
                        if (es->timing)
                                ExplainPropertyFloat("Time", "ms",
-                                                                        INSTR_TIME_GET_MILLISEC(instr->total), 3,
+                                                                        INSTR_TIME_GET_MILLISEC(tginstr->instr.total), 3,
                                                                         es);
-                       ExplainPropertyFloat("Calls", NULL, instr->ntuples, 0, es);
+                       ExplainPropertyInteger("Calls", NULL, tginstr->firings, es);
                }
 
                if (conname)

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 90e94fb8a5a4b531f6f42ae249827f397d0a5514..4d4e96a53023696c8cc5591dd570920b84c8eeb2 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -92,7 +92,7 @@ static bool TriggerEnabled(EState *estate, ResultRelInfo *relinfo,
 static HeapTuple ExecCallTriggerFunc(TriggerData *trigdata,
                                                                         int tgindx,
                                                                         FmgrInfo *finfo,
-                                                                        Instrumentation *instr,
+                                                                        TriggerInstrumentation *instr,
                                                                         MemoryContext per_tuple_context);
 static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
                                                                  ResultRelInfo *src_partinfo,
@@ -2311,7 +2311,7 @@ static HeapTuple
 ExecCallTriggerFunc(TriggerData *trigdata,
                                        int tgindx,
                                        FmgrInfo *finfo,
-                                       Instrumentation *instr,
+                                       TriggerInstrumentation *instr,
                                        MemoryContext per_tuple_context)
 {
        LOCAL_FCINFO(fcinfo, 0);
@@ -2346,7 +2346,7 @@ ExecCallTriggerFunc(TriggerData *trigdata,
         * If doing EXPLAIN ANALYZE, start charging time to this trigger.
         */
        if (instr)
-               InstrStartNode(instr + tgindx);
+               InstrStartTrigger(instr + tgindx);
 
        /*
         * Do the function evaluation in the per-tuple memory context, so that
@@ -2391,10 +2391,10 @@ ExecCallTriggerFunc(TriggerData *trigdata,
 
        /*
         * If doing EXPLAIN ANALYZE, stop charging time to this trigger, and count
-        * one "tuple returned" (really the number of firings).
+        * the firing of the trigger.
         */
        if (instr)
-               InstrStopNode(instr + tgindx, 1);
+               InstrStopTrigger(instr + tgindx, 1);
 
        return (HeapTuple) DatumGetPointer(result);
 }
@@ -3947,7 +3947,7 @@ static void AfterTriggerExecute(EState *estate,
                                                                ResultRelInfo *dst_relInfo,
                                                                TriggerDesc *trigdesc,
                                                                FmgrInfo *finfo,
-                                                               Instrumentation *instr,
+                                                               TriggerInstrumentation *instr,
                                                                MemoryContext per_tuple_context,
                                                                TupleTableSlot *trig_tuple_slot1,
                                                                TupleTableSlot *trig_tuple_slot2);
@@ -4342,7 +4342,7 @@ AfterTriggerExecute(EState *estate,
                                        ResultRelInfo *src_relInfo,
                                        ResultRelInfo *dst_relInfo,
                                        TriggerDesc *trigdesc,
-                                       FmgrInfo *finfo, Instrumentation *instr,
+                                       FmgrInfo *finfo, TriggerInstrumentation *instr,
                                        MemoryContext per_tuple_context,
                                        TupleTableSlot *trig_tuple_slot1,
                                        TupleTableSlot *trig_tuple_slot2)
@@ -4383,7 +4383,7 @@ AfterTriggerExecute(EState *estate,
         * to include time spent re-fetching tuples in the trigger cost.
         */
        if (instr)
-               InstrStartNode(instr + tgindx);
+               InstrStartTrigger(instr + tgindx);
 
        /*
         * Fetch the required tuple(s).
@@ -4600,10 +4600,10 @@ AfterTriggerExecute(EState *estate,
 
        /*
         * If doing EXPLAIN ANALYZE, stop charging time to this trigger, and count
-        * one "tuple returned" (really the number of firings).
+        * the firing of the trigger.
         */
        if (instr)
-               InstrStopNode(instr + tgindx, 1);
+               InstrStopTrigger(instr + tgindx, 1);
 }
 
 
@@ -4719,7 +4719,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
        Relation        rel = NULL;
        TriggerDesc *trigdesc = NULL;
        FmgrInfo   *finfo = NULL;
-       Instrumentation *instr = NULL;
+       TriggerInstrumentation *instr = NULL;
        TupleTableSlot *slot1 = NULL,
                           *slot2 = NULL;
 

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 45e00c6af85de732ed24cdcb29aa93aa3c2dff15..0237d8c3b1d8a76185ffa08146c13a53e8895d64 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1285,7 +1285,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
                resultRelInfo->ri_TrigWhenExprs = (ExprState **)
                        palloc0_array(ExprState *, n);
                if (instrument_options)
-                       resultRelInfo->ri_TrigInstrument = InstrAlloc(n, instrument_options, false);
+                       resultRelInfo->ri_TrigInstrument = InstrAllocTrigger(n, instrument_options);
        }
        else
        {

diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index d35976925ae769ce12bdfbd697dbaadd92f627cd..a2047e4dbc6655b1a3cee9eddaed701549af5e6b 100644 (file)
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -414,7 +414,7 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
 
        /* Set up instrumentation for this node if requested */
        if (estate->es_instrument)
-               result->instrument = InstrAlloc(1, estate->es_instrument,
+               result->instrument = InstrAlloc(estate->es_instrument,
                                                                                result->async_capable);
 
        return result;

diff --git a/src/backend/executor/instrument.c b/src/backend/executor/instrument.c
index a40610bc2522f8d2e671174666f603f8ab70c411..df686ffe048207b1e7ee2479dc0b7654628404bb 100644 (file)
--- a/src/backend/executor/instrument.c
+++ b/src/backend/executor/instrument.c
@@ -26,28 +26,20 @@ static void BufferUsageAdd(BufferUsage *dst, const BufferUsage *add);
 static void WalUsageAdd(WalUsage *dst, WalUsage *add);
 
 
-/* Allocate new instrumentation structure(s) */
+/* Allocate new instrumentation structure */
 Instrumentation *
-InstrAlloc(int n, int instrument_options, bool async_mode)
+InstrAlloc(int instrument_options, bool async_mode)
 {
        Instrumentation *instr;
 
        /* initialize all fields to zeroes, then modify as needed */
-       instr = palloc0(n * sizeof(Instrumentation));
+       instr = palloc0_object(Instrumentation);
        if (instrument_options & (INSTRUMENT_BUFFERS | INSTRUMENT_TIMER | INSTRUMENT_WAL))
        {
-               bool            need_buffers = (instrument_options & INSTRUMENT_BUFFERS) != 0;
-               bool            need_wal = (instrument_options & INSTRUMENT_WAL) != 0;
-               bool            need_timer = (instrument_options & INSTRUMENT_TIMER) != 0;
-               int                     i;
-
-               for (i = 0; i < n; i++)
-               {
-                       instr[i].need_bufusage = need_buffers;
-                       instr[i].need_walusage = need_wal;
-                       instr[i].need_timer = need_timer;
-                       instr[i].async_mode = async_mode;
-               }
+               instr->need_bufusage = (instrument_options & INSTRUMENT_BUFFERS) != 0;
+               instr->need_walusage = (instrument_options & INSTRUMENT_WAL) != 0;
+               instr->need_timer = (instrument_options & INSTRUMENT_TIMER) != 0;
+               instr->async_mode = async_mode;
        }
 
        return instr;
@@ -196,6 +188,32 @@ InstrAggNode(Instrumentation *dst, Instrumentation *add)
                WalUsageAdd(&dst->walusage, &add->walusage);
 }
 
+/* Trigger instrumentation handling */
+TriggerInstrumentation *
+InstrAllocTrigger(int n, int instrument_options)
+{
+       TriggerInstrumentation *tginstr = palloc0_array(TriggerInstrumentation, n);
+       int                     i;
+
+       for (i = 0; i < n; i++)
+               InstrInit(&tginstr[i].instr, instrument_options);
+
+       return tginstr;
+}
+
+void
+InstrStartTrigger(TriggerInstrumentation *tginstr)
+{
+       InstrStartNode(&tginstr->instr);
+}
+
+void
+InstrStopTrigger(TriggerInstrumentation *tginstr, int64 firings)
+{
+       InstrStopNode(&tginstr->instr, 0);
+       tginstr->firings += firings;
+}
+
 /* note current values during parallel executor startup */
 void
 InstrStartParallelQuery(void)

diff --git a/src/include/executor/instrument.h b/src/include/executor/instrument.h
index 9759f3ea5d8d90eb7a6c4504badfd83b72dd11df..be44f629e91f9a6ae2739c7b289a3e2a03845d52 100644 (file)
--- a/src/include/executor/instrument.h
+++ b/src/include/executor/instrument.h
@@ -100,17 +100,28 @@ typedef struct WorkerInstrumentation
        Instrumentation instrument[FLEXIBLE_ARRAY_MEMBER];
 } WorkerInstrumentation;
 
+typedef struct TriggerInstrumentation
+{
+       Instrumentation instr;
+       int64           firings;                /* number of times the instrumented trigger
+                                                                * was fired */
+} TriggerInstrumentation;
+
 extern PGDLLIMPORT BufferUsage pgBufferUsage;
 extern PGDLLIMPORT WalUsage pgWalUsage;
 
-extern Instrumentation *InstrAlloc(int n, int instrument_options,
-                                                                  bool async_mode);
+extern Instrumentation *InstrAlloc(int instrument_options, bool async_mode);
 extern void InstrInit(Instrumentation *instr, int instrument_options);
 extern void InstrStartNode(Instrumentation *instr);
 extern void InstrStopNode(Instrumentation *instr, double nTuples);
 extern void InstrUpdateTupleCount(Instrumentation *instr, double nTuples);
 extern void InstrEndLoop(Instrumentation *instr);
 extern void InstrAggNode(Instrumentation *dst, Instrumentation *add);
+
+extern TriggerInstrumentation *InstrAllocTrigger(int n, int instrument_options);
+extern void InstrStartTrigger(TriggerInstrumentation *tginstr);
+extern void InstrStopTrigger(TriggerInstrumentation *tginstr, int64 firings);
+
 extern void InstrStartParallelQuery(void);
 extern void InstrEndParallelQuery(BufferUsage *bufusage, WalUsage *walusage);
 extern void InstrAccumParallelQuery(BufferUsage *bufusage, WalUsage *walusage);

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 090cfccf65fa016d127739fad1bf7373c78f87dc..908898aa7c9e5ee086d67a528886bec2cb2a6410 100644 (file)
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -60,6 +60,7 @@ typedef struct ScanKeyData ScanKeyData;
 typedef struct SnapshotData *Snapshot;
 typedef struct SortSupportData *SortSupport;
 typedef struct TIDBitmap TIDBitmap;
+typedef struct TriggerInstrumentation TriggerInstrumentation;
 typedef struct TupleConversionMap TupleConversionMap;
 typedef struct TupleDescData *TupleDesc;
 typedef struct Tuplesortstate Tuplesortstate;
@@ -552,7 +553,7 @@ typedef struct ResultRelInfo
        ExprState **ri_TrigWhenExprs;
 
        /* optional runtime measurements for triggers */
-       Instrumentation *ri_TrigInstrument;
+       TriggerInstrumentation *ri_TrigInstrument;
 
        /* On-demand created slots for triggers / returning processing */
        TupleTableSlot *ri_ReturningSlot;       /* for trigger output tuples */

diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 0c5493bd47f35581337a11a17d5b22102e68c412..6a328fceaeeba84817ccf4e09af055aa6670c51e 100644 (file)
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3215,6 +3215,7 @@ TriggerDesc
 TriggerEvent
 TriggerFlags
 TriggerInfo
+TriggerInstrumentation
 TriggerTransition
 TruncateStmt
 TsmRoutine