proposal - queryid can be used as filter for auto_explain

Hi,

On Mon, Apr 13, 2026 at 06:46:24AM +0200, Pavel Stehule wrote:

I have to investigate strange behaviour of some specific query. One
possibility is non stability of plan. This is a bigger server with a bigger
load, and I am a little bit afraid to use auto_explain for all queries.

Currently auto_explain doesn't support it, but it can be practical if we
can specify a list of queryid as a filter of auto_explain.

What do you think about this idea?

+1. I don't think it's something that will be useful very often, but when it's
going to be useful it would be *extremely* convenient to have.

On Mon, Apr 13, 2026 at 11:02 AM Julien Rouhaud <rjuju123@gmail.com> wrote:

Hi,

On Mon, Apr 13, 2026 at 06:46:24AM +0200, Pavel Stehule wrote:

I have to investigate strange behaviour of some specific query. One
possibility is non stability of plan. This is a bigger server with a bigger
load, and I am a little bit afraid to use auto_explain for all queries.

Currently auto_explain doesn't support it, but it can be practical if we
can specify a list of queryid as a filter of auto_explain.

What do you think about this idea?

+1. I don't think it's something that will be useful very often, but when it's
going to be useful it would be *extremely* convenient to have.

+1 too.

The only doubt would be: if that's new GUC, then this is list of queryIDs,
right? So do you plan also logging the queryid from auto_explain directly or
one should have %Q in log_line_prefix?

-J.

st 15. 4. 2026 v 8:37 odesílatel Jakub Wartak <jakub.wartak@enterprisedb.com>
napsal:

On Mon, Apr 13, 2026 at 11:02 AM Julien Rouhaud <rjuju123@gmail.com>
wrote:

Hi,

On Mon, Apr 13, 2026 at 06:46:24AM +0200, Pavel Stehule wrote:

I have to investigate strange behaviour of some specific query. One
possibility is non stability of plan. This is a bigger server with a

bigger

load, and I am a little bit afraid to use auto_explain for all queries.

Currently auto_explain doesn't support it, but it can be practical if

we

can specify a list of queryid as a filter of auto_explain.

What do you think about this idea?

+1. I don't think it's something that will be useful very often, but

when it's

going to be useful it would be *extremely* convenient to have.

+1 too.

The only doubt would be: if that's new GUC, then this is list of queryIDs,
right? So do you plan also logging the queryid from auto_explain directly
or
one should have %Q in log_line_prefix?

I plan a new GUC with a list of values. I haven't thought about
implementation yet. I'll send a prototype next week.

Regards

Pavel

Hi

st 15. 4. 2026 v 9:39 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

st 15. 4. 2026 v 8:37 odesílatel Jakub Wartak <
jakub.wartak@enterprisedb.com> napsal:

On Mon, Apr 13, 2026 at 11:02 AM Julien Rouhaud <rjuju123@gmail.com>
wrote:

Hi,

On Mon, Apr 13, 2026 at 06:46:24AM +0200, Pavel Stehule wrote:

I have to investigate strange behaviour of some specific query. One
possibility is non stability of plan. This is a bigger server with a

bigger

load, and I am a little bit afraid to use auto_explain for all

queries.

Currently auto_explain doesn't support it, but it can be practical if

we

can specify a list of queryid as a filter of auto_explain.

What do you think about this idea?

+1. I don't think it's something that will be useful very often, but

when it's

going to be useful it would be *extremely* convenient to have.

+1 too.

The only doubt would be: if that's new GUC, then this is list of queryIDs,
right? So do you plan also logging the queryid from auto_explain directly
or
one should have %Q in log_line_prefix?

I plan a new GUC with a list of values. I haven't thought about
implementation yet. I'll send a prototype next week.

Regards

Pavel

Today I finished the patch.

Regards

Pavel

Hi

small change - parsing queryid by usage pg_strtoint64_safe instead strtol

Regards

Pavel

Hi Pavel,

On Wed, Jun 17, 2026 at 9:21 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

small change - parsing queryid by usage pg_strtoint64_safe instead strtol

+       DefineCustomStringVariable("auto_explain.log_queryids",
+                                                          "Only
queries with qyeryid from list will be logged.",

Other than this 'qyeryid' typo I don't see anything obviously wrong. Well
maybe if we are scared about too heavy impact of scanning too long array
of queryids, we could enforce some max limitation there on the count
accepted (but I'm not sure if it is worth it).

+ if (log_filter && msec >= auto_explain_log_min_duration)

With QA hat on: maybe it would be possible to emit some warning message
if log_queryids is set to anything and auto_explain_log_min_duration is
at the default of -1? (because that way nothing gets logged). My first
impression when using was If I would enable it .log_queryids would work
out of the box, but of course due to this line of code both criteria
need to be meet, but emitting some HINT when just .log_queryids would be
set would be more user firendly (yes, it's covered in the docs, but people
rarely check it).

Anyway, it looks like very useful addition to me.

-J.

Hi

čt 18. 6. 2026 v 13:38 odesílatel Jakub Wartak <
jakub.wartak@enterprisedb.com> napsal:

Hi Pavel,

On Wed, Jun 17, 2026 at 9:21 AM Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

small change - parsing queryid by usage pg_strtoint64_safe instead strtol

+       DefineCustomStringVariable("auto_explain.log_queryids",
+                                                          "Only
queries with qyeryid from list will be logged.",

Other than this 'qyeryid' typo I don't see anything obviously wrong. Well
maybe if we are scared about too heavy impact of scanning too long array
of queryids, we could enforce some max limitation there on the count
accepted (but I'm not sure if it is worth it).

+ if (log_filter && msec >= auto_explain_log_min_duration)

With QA hat on: maybe it would be possible to emit some warning message
if log_queryids is set to anything and auto_explain_log_min_duration is
at the default of -1? (because that way nothing gets logged). My first
impression when using was If I would enable it .log_queryids would work
out of the box, but of course due to this line of code both criteria
need to be meet, but emitting some HINT when just .log_queryids would be
set would be more user firendly (yes, it's covered in the docs, but people
rarely check it).

Unfortunately, I don't know when this warning should be raised. If I raise
when I parse GUC, then
the warning will be raised everywhere, when users use SET or set_config
function (because,
only once GUC can be changed at one time). If I raise a warning at executor
end time, then
there is a risk of a log storm - this is what I exactly don't want.

An alternative can be implementation when log_queryids and log_min_duration
are independent constraints. Then there is not a problem that you describe.
But from
practical usage, I think current implementation is better - sometimes we
want only specific
plans longer than specified time. There can be another GUC, that can
specify the relation
between log_queryids and log_min_duration - but at this time it looks like
overengineering.

So warning at documentation looks like the lesser of two evils.

Thank you for check

Nice evening

Pavel

Hello

+ PGC_SUSET | GUC_LIST_INPUT,
+ 0,

Shouldn't that be PGC_SUSET, GUC_LIST_INPUT?

+	/*
+	 * In almost all cases, the queryid is computed due pg_stat_statements.
+	 * Without log_queryids computing queryid is not necessary, but it can
+	 * be hard to enable or disable queryid in dependecy of log_queryids.
+	 * There are two possibilities - force queryid computing, or ignore
+	 * queries without computed queryid (computing should be forced by setting
+	 * compute_query_id). Boths probably can work, first looks more clean
+	 * at this moment.
+	 */
+	EnableQueryId();

This comment seems misleading to me. Based on it, I would think that we always force queryid computing, but EnableQueryId doesn't do anything with compute_query_id = off, so it seems like the second choice instead? (also typo: Boths)

+	result = (auto_explain_queryids *) guc_malloc(LOG, allocsize);
+	if (result == NULL)
+		return false;

This leaks rawstring/elemlist, is that intentional?

+ if (*newval == NULL || *newval[0] == '\0')

That should be probably (*newval)[0].

Hi

čt 18. 6. 2026 v 23:01 odesílatel Zsolt Parragi <zsolt.parragi@percona.com>
napsal:

Hello

+                                                          PGC_SUSET |
GUC_LIST_INPUT,
+                                                          0,

Shouldn't that be PGC_SUSET, GUC_LIST_INPUT?

yes, fixed

+       /*
+        * In almost all cases, the queryid is computed due
pg_stat_statements.
+        * Without log_queryids computing queryid is not necessary, but it
can
+        * be hard to enable or disable queryid in dependecy of
log_queryids.
+        * There are two possibilities - force queryid computing, or ignore
+        * queries without computed queryid (computing should be forced by
setting
+        * compute_query_id). Boths probably can work, first looks more
clean
+        * at this moment.
+        */
+       EnableQueryId();

I was not sure if we need to "force" queryid commuting, because usually it
will be computed due pg_stat_statements.
computing queryid is critical for pg_stat_statements, auto_explain can work
without it - only log_queryids will be ineffective.
But now I am sure, so EnableQueryId should be used in auto_explain too. For
consistency, for testing independence.
At the end - users can set compute_query_id to off.

I changed this comment to the same form as other usage of EnableQueryId.

This comment seems misleading to me. Based on it, I would think that
we always force queryid computing, but EnableQueryId doesn't do
anything with compute_query_id = off, so it seems like the second
choice instead? (also typo: Boths)

+       result = (auto_explain_queryids *) guc_malloc(LOG, allocsize);
+       if (result == NULL)
+               return false;

This leaks rawstring/elemlist, is that intentional?

Boths are allocated from shortlife memory context, and will be released
early. So there is no real risk of memory leaks.

+ if (*newval == NULL || *newval[0] == '\0')

That should be probably (*newval)[0].

I removed this - SplitGUCList can process empty strings. I leave this
routine, when I detect a zero element list.

Thank you for check and comments

Regards

Pavel