Hi
st 1. 7. 2026 v 4:30 odesÃlatel Kyotaro Horiguchi <[email protected]>
napsal:
> Hello, I have a few comments on this.
>
> + /*
> + * Inform the postmaster that we want to enable query_id
> calculation if
> + * compute_query_id is set to auto.
> + */
> + EnableQueryId();
>
> This comment is not entirely accurate because when the module is
> loaded using LOAD, the setting is enabled only for the current
> session, not in the postmaster.
>
This comment was taken from pg_stat_statements that cannot be loaded.
I changed the comment to /* If compute_query_id = 'auto', we would like
query IDs. */
This sentence is used by pg_stash_advice.c
>
> Separately from that, I don't think auto_explain should enable query
> ID calculation when it is not actually going to use it. Instead, I
> wonder whether this should be done in assign_log_queryids(). This
> would naturally tie enabling query ID calculation to the state of the
> GUC, including configuration reloads, as well as session-local use via
> LOAD, while avoiding enabling it in sessions that never use
> auto_explain.
>
This is a hard question - I am not sure if one GUC can force a second GUC.
GUC are transactional and stacked, and afraid about cleaning in this case.
I think so any
feature can depend on more GUC, but in this case, the dependency is on
external modules, and then this can be hard to implement without some new
hook.
In this case I prefer simplicity - it is the same way as
pg_stat_statements. It doesn't
need queryid by default, and still it enables queryid by default. I think
it is most
user friendly for users that try to use queryid_filter. At the end, almost
all users
uses pg_stat_statements - so queryid will be enabled, and just for special
cases,
looks to me better to enable queryid explicitly in auto_explain too. It can
be disabled
any time explicitly - and then it is clear, so queryid_filter cannot work.
So I understand to
your comment, but choosed behave looks like most robust - and other ways
like
more fragile
Please, check Jakub Wartak comment - I am afraid to raise log when queryid
is not calculated and queryid_filter is not empty. I am afraid to force
queryid calculation from assigning_queryids_filter. So the implemented
design is the last way. Instead forcing queryid calculation in
queryids_filter assignment is better generally not forcing queryid
calculation.
>
>
> + if (queryId != INT64CONST(0))
> + {
> + int i;
> +
> ...
> + for (i = 0; i < queryId_filter->nqueryids; i++)
> + {
>
> I think it would be better to write this as "for (int i = ...".
>
>
changed
> - if (msec >= auto_explain_log_min_duration)
> + if (log_filter && msec >= auto_explain_log_min_duration)
>
> I think the overall decision logic for deciding whether to log a plan
> could be reorganized to avoid unnecessary query ID lookups.
>
when queryids_filter will be empty, then log_filter will be true.
Anytime, the user can set compute_queryid to OFF. My idea is - it should
work by default, and when the user doesn't want it, then he can explicitly
disable it.
>
>
> + /*
> + * We expect small number of watched queryids, and then
> + * a linear seaching is the fastest. As an alternative
> + * we can sort the array of queryId, and we can search
> + * there by bisection.
> + */
>
> I also think a linear search is fine here. If this ever turns out to
> be insufficient, I expect we'd simply switch to a more appropriate
> algorithm, regardless of whether the alternatives are mentioned here
> or not. Also, "is the fastest" seems stronger than necessary. I think
> we usually write comments like:
>
> > We expect only a few watched query IDs, so a linear search is sufficient.
>
> changed
>
> + if (log_filter && msec >= auto_explain_log_min_duration)
>
> This part made me pause because the relationship between log_filter
> and log_min_duration is not immediately obvious.
>
> Initially, I assumed they were independent options, so I expected an
> OR relationship. However, the implementation uses AND. If users also
> assume these are independent settings, the behavior may be
> surprising. Perhaps it would make sense to introduce a separate
> log_queryid_min_duration parameter if these are intended to be
> independent controls.
>
I thought about this possibility. But I think thinking about log_queryid
and log_min_duration as independent constraints can be confusing too
(and less safe). The log_queryid in this case is stronger filter.
At this moment I don't want to introduce a second new GUC, and without
GUC (like you proposed), the implemented design looks less confusing
and more practical. The possibility of having two (three) variants of
log_min_duration looks strange to me.
I like to think log_queryid is a second constraint, not a new feature.
>
> + /*
> + * queryid is 64bit integer. strtol fails on 32bit or on MSWin.
> + * In other places, queryid passed (from, to SQL) as generic int8,
> + * so we can use int8in routine. int8in can raise an exception,
> + * so pg_strtoint64_safe is used instead.
> + */
>
> I don't think this comment is necessary. Using pg_strtoint64_safe()
> already makes the intent clear.
>
removed
>
> I also think it would be good to add a test for the LOAD case.
>
What do you want to test in this case?
Thank you for check and comments
modified patch attached
Pavel
>
> Regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
From d96785a8777985349089426c09b9051ee8c750b6 Mon Sep 17 00:00:00 2001
From: "[email protected]" <[email protected]>
Date: Sun, 14 Jun 2026 19:43:04 +0200
Subject: [PATCH] auto_explain.log_queryids
this patch implements new auto_explain configuration - auto_explain.log_queryids.
It allow to specify list of queryid. Only plans of queries with queryid from
specified list will be logged. This allows to use auto_explain in higher load
environment for monitoring problematic or extra important queries with lower
risk of high log bloating.
---
contrib/auto_explain/auto_explain.c | 150 ++++++++++++++++++++-
contrib/auto_explain/t/001_auto_explain.pl | 53 ++++++++
doc/src/sgml/auto-explain.sgml | 20 +++
3 files changed, 222 insertions(+), 1 deletion(-)
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 97ce498d0c1..9485a9c9d01 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -22,8 +22,10 @@
#include "common/pg_prng.h"
#include "executor/instrument.h"
#include "nodes/makefuncs.h"
+#include "nodes/queryjumble.h"
#include "nodes/value.h"
#include "parser/scansup.h"
+#include "utils/builtins.h"
#include "utils/guc.h"
#include "utils/varlena.h"
@@ -48,6 +50,7 @@ static int auto_explain_log_level = LOG;
static bool auto_explain_log_nested_statements = false;
static double auto_explain_sample_rate = 1;
static char *auto_explain_log_extension_options = NULL;
+static char *auto_explain_log_queryids = NULL;
/*
* Parsed form of one option from auto_explain.log_extension_options.
@@ -81,6 +84,17 @@ static const struct config_enum_entry format_options[] = {
{NULL, 0, false}
};
+/*
+ * parsed form of auto_explain.log_queryids.
+ */
+typedef struct auto_explain_queryids
+{
+ int nqueryids;
+ int64 queryid[FLEXIBLE_ARRAY_MEMBER];
+} auto_explain_queryids;
+
+static auto_explain_queryids *queryid_filter = NULL;
+
static const struct config_enum_entry loglevel_options[] = {
{"debug5", DEBUG5, false},
{"debug4", DEBUG4, false},
@@ -129,6 +143,9 @@ static int auto_explain_split_options(char *rawstring,
auto_explain_option *options,
int maxoptions, char **errmsg);
+static bool check_log_queryids(char **newval, void **extra, GucSource source);
+static void assign_log_queryids(const char *newval, void *extra);
+
/*
* Module load callback
*/
@@ -307,6 +324,16 @@ _PG_init(void)
NULL,
NULL);
+ DefineCustomStringVariable("auto_explain.log_queryids",
+ "Only queries with queryid from list will be logged.",
+ NULL,
+ &auto_explain_log_queryids,
+ NULL,
+ PGC_SUSET, GUC_LIST_INPUT,
+ check_log_queryids,
+ assign_log_queryids,
+ NULL);
+
MarkGUCPrefixReserved("auto_explain");
/* Install hooks. */
@@ -318,6 +345,9 @@ _PG_init(void)
ExecutorFinish_hook = explain_ExecutorFinish;
prev_ExecutorEnd = ExecutorEnd_hook;
ExecutorEnd_hook = explain_ExecutorEnd;
+
+ /* If compute_query_id = 'auto', we would like query IDs. */
+ EnableQueryId();
}
/*
@@ -424,6 +454,7 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
{
MemoryContext oldcxt;
double msec;
+ bool log_filter = true;
/*
* Make sure we operate in the per-query context, so any cruft will be
@@ -431,9 +462,33 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
*/
oldcxt = MemoryContextSwitchTo(queryDesc->estate->es_query_cxt);
+ if (queryid_filter)
+ {
+ int64 queryid = queryDesc->plannedstmt->queryId;
+
+ /* check if current queryID is in watched queryIds */
+ log_filter = false;
+
+ if (queryid != INT64CONST(0))
+ {
+ /*
+ * We expect only a few watched query IDs, so a linear search
+ * is sufficient.
+ */
+ for (int i = 0; i < queryid_filter->nqueryids; i++)
+ {
+ if (queryid_filter->queryid[i] == queryid)
+ {
+ log_filter = true;
+ break;
+ }
+ }
+ }
+ }
+
/* Log plan if duration is exceeded. */
msec = INSTR_TIME_GET_MILLISEC(queryDesc->query_instr->total);
- if (msec >= auto_explain_log_min_duration)
+ if (log_filter && msec >= auto_explain_log_min_duration)
{
ExplainState *es = NewExplainState();
@@ -827,3 +882,96 @@ auto_explain_split_options(char *rawstring, auto_explain_option *options,
return noptions;
}
+
+/*
+ * log_queryids
+ */
+static bool
+check_log_queryids(char **newval, void **extra, GucSource source)
+{
+ char *rawstring;
+ List *elemlist = NIL;
+ ListCell *l;
+ Size allocsize;
+ auto_explain_queryids *result;
+ int i = 0;
+
+ if (*newval == NULL)
+ {
+ *extra = NULL;
+ return true;
+ }
+
+ /* need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ if (!SplitGUCList(rawstring, ',', &elemlist))
+ {
+ pfree(rawstring);
+ list_free(elemlist);
+
+ /* syntax error in list */
+ GUC_check_errdetail("List syntax is invalid.");
+ return false;
+ }
+
+ if (list_length(elemlist) == 0)
+ {
+ pfree(rawstring);
+ list_free(elemlist);
+
+ *extra = NULL;
+ return true;
+ }
+
+ /* Try to allocate an auto_explain_queryids object. */
+ allocsize = offsetof(auto_explain_queryids, queryid) +
+ sizeof(int64) * list_length(elemlist);
+
+ result = (auto_explain_queryids *) guc_malloc(LOG, allocsize);
+ if (result == NULL)
+ {
+ pfree(rawstring);
+ list_free(elemlist);
+
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ char *tok = (char *) lfirst(l);
+ int64 queryid;
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+
+ queryid = pg_strtoint64_safe(tok, (Node *) &escontext);
+ if (escontext.error_occurred || queryid == INT64CONST(0))
+ {
+ GUC_check_errmsg("queryId \"%s\" is not valid", tok);
+
+ pfree(rawstring);
+ list_free(elemlist);
+ guc_free(result);
+
+ return false;
+ }
+
+ result->queryid[i++] = queryid;
+ }
+
+ result->nqueryids = i;
+
+ pfree(rawstring);
+ list_free(elemlist);
+
+ *extra = result;
+
+ return true;
+}
+
+static void
+assign_log_queryids(const char *newval, void *extra)
+{
+ auto_explain_queryids *myextra = (auto_explain_queryids *) extra;
+
+ queryid_filter = myextra;
+}
diff --git a/contrib/auto_explain/t/001_auto_explain.pl b/contrib/auto_explain/t/001_auto_explain.pl
index f32a5e84f65..148e83f7545 100644
--- a/contrib/auto_explain/t/001_auto_explain.pl
+++ b/contrib/auto_explain/t/001_auto_explain.pl
@@ -7,6 +7,7 @@ use warnings FATAL => 'all';
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+use JSON::PP;
# Runs the specified query and returns the emitted server log.
# params is an optional hash mapping GUC names to values;
@@ -27,6 +28,15 @@ sub query_log
return slurp_file($log, $offset);
}
+sub get_queryid
+{
+ my ($node, $sql) = @_;
+
+ my $plan = $node->safe_psql('postgres', 'EXPLAIN (VERBOSE, FORMAT JSON) ' . $sql);
+
+ return int(decode_json($plan)->[0]->{"Query Identifier"});
+}
+
my $node = PostgreSQL::Test::Cluster->new('main');
$node->init(auth_extra => [ '--create-role' => 'regress_user1' ]);
$node->append_conf('postgresql.conf',
@@ -239,4 +249,47 @@ WHERE module_name = 'auto_explain';
});
like($res, qr/^auto_explain\|t\|auto_explain$/, "pg_get_loaded_modules() ok");
+# test detection of broken format
+$log_contents = query_log($node, "SELECT 1;",
+ { "auto_explain.log_queryids" => "x" });
+
+like(
+ $log_contents,
+ qr/queryId "x" is not valid/,
+ "invalid queryid detected");
+
+my $query1 = "SELECT * FROM pg_class LIMIT 1";
+my $query2 = "SELECT * FROM pg_proc LIMIT 1";
+my $query3 = "SELECT * FROM pg_namespace LIMIT 1";
+
+my $queryid1 = get_queryid($node, $query1);
+my $queryid2 = get_queryid($node, $query2);
+my $queryid3 = get_queryid($node, $query3);
+
+my $qids = "$queryid1,$queryid2";
+
+$log_contents = query_log($node, $query1,
+ { "auto_explain.log_queryids" => $qids });
+
+like(
+ $log_contents,
+ qr/Query Text: SELECT \* FROM pg_class LIMIT 1/,
+ "plan for query specified by queryid found");
+
+$log_contents = query_log($node, $query2,
+ { "auto_explain.log_queryids" => $qids });
+
+like(
+ $log_contents,
+ qr/Query Text: SELECT \* FROM pg_proc LIMIT 1/,
+ "plan for query specified by queryid found");
+
+$log_contents = query_log($node, $query3,
+ { "auto_explain.log_queryids" => $qids });
+
+unlike(
+ $log_contents,
+ qr/Query Text: SELECT \* FROM pg_namespace LIMIT 1/,
+ "plan for query with disallowed queryid not found");
+
done_testing();
diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml
index 06a8fcc6c5b..8fe136472bc 100644
--- a/doc/src/sgml/auto-explain.sgml
+++ b/doc/src/sgml/auto-explain.sgml
@@ -63,6 +63,26 @@ LOAD 'auto_explain';
</listitem>
</varlistentry>
+ <varlistentry id="auto-explain-configuration-parameters-log-queryids">
+ <term>
+ <varname>auto_explain.log_queryids</varname> (<type>list of integers</type>)
+ <indexterm>
+ <primary><varname>auto_explain.log_queryids</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ <varname>auto_explain.log_queryids</varname> allows to specify list of
+ query ID. Only plans with listed ID will be logged. When this parameter
+ is empty (default), the query ID is not used for filtering. This doesn't
+ disable <varname>auto_explain.log_min_duration</varname>.
+ <varname>auto_explain.log_queryids</varname> and
+ <varname>auto_explain.log_min_duration</varname> work together. Only
+ superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="auto-explain-configuration-parameters-log-parameter-max-length">
<term>
<varname>auto_explain.log_parameter_max_length</varname> (<type>integer</type>)
--
2.54.0
