From d444dcdd48f5712cfcae7e7f4cc8055f1c33f902 Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 9 Sep 2025 02:16:59 -0700 Subject: [PATCH v15 4/9] instrumentation: Allocate queryDesc->totaltime in ExecutorStart if needed This introduces a new field, queryDesc->totaltime_options, that extensions can use to indicate whether they need queryDesc->totaltime populated, and with which instrumentation options. Extensions should take care to only add options they need, instead of replacing the options of others. This replaces the practice of extensions allocating queryDesc->totaltime themselves, which required them to always use INSTRUMENT_ALL for the options argument. If they wouldn't have, another extension could silently be impacted by it. It also unnecessarily made extensions hooks worry about being sure to allocate in the per-query memory context. Adjust pg_stat_statements and auto_explain to match, and lower the requested instrumentation level for auto_explain to INSTRUMENT_TIMER, since the summary instrumentation it needs is only runtime. Author: Lukas Fittl Reviewed-by: Discussion: --- contrib/auto_explain/auto_explain.c | 20 +++------------ .../pg_stat_statements/pg_stat_statements.c | 25 ++++++------------- src/backend/executor/execMain.c | 9 +++++++ src/backend/tcop/pquery.c | 1 + src/include/executor/execdesc.h | 4 ++- 5 files changed, 23 insertions(+), 36 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index 39bf2543b70..2f882026b50 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -284,6 +284,9 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags) if (auto_explain_enabled()) { + /* We're always interested in runtime */ + queryDesc->totaltime_options |= INSTRUMENT_TIMER; + /* Enable per-node instrumentation iff log_analyze is required. */ if (auto_explain_log_analyze && (eflags & EXEC_FLAG_EXPLAIN_ONLY) == 0) { @@ -302,23 +305,6 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags) prev_ExecutorStart(queryDesc, eflags); else standard_ExecutorStart(queryDesc, eflags); - - if (auto_explain_enabled()) - { - /* - * Set up to track total elapsed time in ExecutorRun. Make sure the - * space is allocated in the per-query context so it will go away at - * ExecutorEnd. - */ - if (queryDesc->totaltime == NULL) - { - MemoryContext oldcxt; - - oldcxt = MemoryContextSwitchTo(queryDesc->estate->es_query_cxt); - queryDesc->totaltime = InstrAlloc(INSTRUMENT_ALL); - MemoryContextSwitchTo(oldcxt); - } - } } /* diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index b6863479e9f..346adb5599f 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -983,11 +983,6 @@ pgss_planner(Query *parse, static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags) { - if (prev_ExecutorStart) - prev_ExecutorStart(queryDesc, eflags); - else - standard_ExecutorStart(queryDesc, eflags); - /* * If query has queryId zero, don't track it. This prevents double * counting of optimizable statements that are directly contained in @@ -995,20 +990,14 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags) */ if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != INT64CONST(0)) { - /* - * Set up to track total elapsed time in ExecutorRun. Make sure the - * space is allocated in the per-query context so it will go away at - * ExecutorEnd. - */ - if (queryDesc->totaltime == NULL) - { - MemoryContext oldcxt; - - oldcxt = MemoryContextSwitchTo(queryDesc->estate->es_query_cxt); - queryDesc->totaltime = InstrAlloc(INSTRUMENT_ALL); - MemoryContextSwitchTo(oldcxt); - } + /* Request all summary instrumentation, i.e. timing, buffers and WAL */ + queryDesc->totaltime_options |= INSTRUMENT_ALL; } + + if (prev_ExecutorStart) + prev_ExecutorStart(queryDesc, eflags); + else + standard_ExecutorStart(queryDesc, eflags); } /* diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index b0f636bf8b6..7d74f6da402 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -250,6 +250,15 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags) estate->es_instrument = queryDesc->instrument_options; estate->es_jit_flags = queryDesc->plannedstmt->jitFlags; + /* + * Set up query-level instrumentation if extensions have requested it via + * totaltime_options. Ensure an extension has not allocated totaltime + * itself. + */ + Assert(queryDesc->totaltime == NULL); + if (queryDesc->totaltime_options) + queryDesc->totaltime = InstrQueryAlloc(queryDesc->totaltime_options); + /* * Set up an AFTER-trigger statement context, unless told not to, or * unless it's EXPLAIN-only mode (when ExecutorFinish won't be called). diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index d8fc75d0bb9..e27f26ecd83 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -86,6 +86,7 @@ CreateQueryDesc(PlannedStmt *plannedstmt, qd->params = params; /* parameter values passed into query */ qd->queryEnv = queryEnv; qd->instrument_options = instrument_options; /* instrumentation wanted? */ + qd->totaltime_options = 0; /* null these fields until set by ExecutorStart */ qd->tupDesc = NULL; diff --git a/src/include/executor/execdesc.h b/src/include/executor/execdesc.h index d3a57242844..0d76a1c173e 100644 --- a/src/include/executor/execdesc.h +++ b/src/include/executor/execdesc.h @@ -42,6 +42,8 @@ typedef struct QueryDesc ParamListInfo params; /* param values being passed in */ QueryEnvironment *queryEnv; /* query environment passed in */ int instrument_options; /* OR of InstrumentOption flags */ + int totaltime_options; /* OR of InstrumentOption flags for + * totaltime */ /* These fields are set by ExecutorStart */ TupleDesc tupDesc; /* descriptor for result tuples */ @@ -51,7 +53,7 @@ typedef struct QueryDesc /* This field is set by ExecutePlan */ bool already_executed; /* true if previously executed */ - /* This is always set NULL by the core system, but plugins can change it */ + /* This field is allocated by ExecutorRun if needed */ struct Instrumentation *totaltime; /* total time spent in ExecutorRun */ } QueryDesc; -- 2.47.1