Horton
Menu

Thread

  1. proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-04-13T04:46:24Z

    Hi

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?

Regards

Pavel

  2. Re: proposal - queryid can be used as filter for auto_explain

    Julien Rouhaud <rjuju123@gmail.com> — 2026-04-13T09:01:47Z

    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.

  3. Re: proposal - queryid can be used as filter for auto_explain

    Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-15T06:37:48Z

    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.

  4. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-04-15T07:39:28Z

    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




> -J.
>

  5. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-06-16T05:55:47Z

    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

>
>
>
>> -J.
>>
>

  6. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-06-17T07:20:52Z

    Hi

small change - parsing queryid by usage pg_strtoint64_safe instead strtol

Regards

Pavel

  7. Re: proposal - queryid can be used as filter for auto_explain

    Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-06-18T11:38:23Z

    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.

  8. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-06-18T19:47:30Z

    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



> Anyway, it looks like very useful addition to me.
>
> -J.
>

  9. Re: proposal - queryid can be used as filter for auto_explain

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-06-18T21:01:47Z

    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].

  10. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-06-19T06:05:52Z

    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

  11. Re: proposal - queryid can be used as filter for auto_explain

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-06-30T22:19:32Z

    Thanks, the changes look good to me, I only have some minor comments
about naming/grammar:

+
+static auto_explain_queryids *queryId_filter = NULL;
+

That queryId_filter mixed case name seems strange, maybe
queryid_filter would be better?

+				/*
+				 * 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.
+				 */

The patch should do either of those, but I don't think this comment
should stay in it after that decision? Linear search seems to be the
simpler/better choice to me, unless the goal is to handle 32-64-or
more ids.

+	/* Try to allocate an auto_explain_extension_options object. */
+	allocsize = offsetof(auto_explain_queryids, queryId) +
+		sizeof(int64) * list_length(elemlist);

The type is auto_explain_queryids, auto_explain_extension_options
isn't mentioned anywhere else?

"allows to specify list of queryid" - maybe query IDs?
"Only plan of query with queryid" - same
"Only queries with queryId from list will be logged." - same issue,
maybe "Only queries with the listed IDs will be logged?"
"works together" - work together

  12. Re: proposal - queryid can be used as filter for auto_explain

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2026-07-01T02:30:26Z

    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.

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.


+           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 = ...".

-		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.


+           /*
+            * 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.


+               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.


+        /*
+         * 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.

I also think it would be good to add a test for the LOAD case.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  13. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-07-01T18:21:24Z

    Hi

st 1. 7. 2026 v 0:19 odesílatel Zsolt Parragi <zsolt.parragi@percona.com>
napsal:

> Thanks, the changes look good to me, I only have some minor comments
> about naming/grammar:
>
> +
> +static auto_explain_queryids *queryId_filter = NULL;
> +
>

That queryId_filter mixed case name seems strange, maybe
> queryid_filter would be better?
>

changed

Unfortunately, there is not name consistency already for queryid (without
this patch) - somewhere is queryid elsewhere is queryId


>
> +                               /*
> +                                * 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.
> +                                */
>
> The patch should do either of those, but I don't think this comment
> should stay in it after that decision? Linear search seems to be the
> simpler/better choice to me, unless the goal is to handle 32-64-or
> more ids.
>

ok, I removed this comment


>
> +       /* Try to allocate an auto_explain_extension_options object. */
> +       allocsize = offsetof(auto_explain_queryids, queryId) +
> +               sizeof(int64) * list_length(elemlist);
>
> The type is auto_explain_queryids, auto_explain_extension_options
> isn't mentioned anywhere else?
>

yes - it is copy/paste error, fixed


>
> "allows to specify list of queryid" - maybe query IDs?
> "Only plan of query with queryid" - same
> "Only queries with queryId from list will be logged." - same issue,
> maybe "Only queries with the listed IDs will be logged?"
> "works together" - work together
>

changed

Thank you for review

  14. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-07-01T19:51:30Z

    Hi

st 1. 7. 2026 v 4:30 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com>
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
>

  15. Re: proposal - queryid can be used as filter for auto_explain

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-07-01T22:08:54Z

    +      <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

I would still improve this part a bit, ID is singular at both places
but it should be plural IDs, and there's a double space at "this
parameter".

Other than these I have no more comments, the patch looks good to me.

  16. Re: proposal - queryid can be used as filter for auto_explain

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2026-07-02T03:01:44Z

    Hello,

At Wed, 1 Jul 2026 21:51:30 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in 
> > 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.

I think there may be a misunderstanding.

I was not suggesting that one GUC should force another GUC. My point
was about when auto_explain calls EnableQueryId(), not about changing
the value of compute_query_id.

Instead of requesting query ID calculation unconditionally when the
module is loaded, I was wondering whether EnableQueryId() could be
called from assign_log_queryids(), so that auto_explain only requests
query ID calculation when it is actually going to use it.

> 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.

I think that is a slightly different issue.

I was not suggesting that queryids_filter itself should force query ID
calculation, nor that we should emit a warning when query ID is not
available.

My point was that, if log_queryids is the option that makes
auto_explain use the query ID, then auto_explain could call
EnableQueryId() when that option is enabled. In that case, the
situation where queryids_filter is set but no query ID is calculated
would not be introduced by this change; if log_queryids is off,
auto_explain is not going to use the query ID anyway.

> > -               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.

Yes, that behavior makes sense. I was not suggesting changing the
meaning of an empty queryids_filter.

My comment was about the ordering of the checks in the proposed patch.
If queryids_filter is empty, no query ID lookup is needed to decide
whether the query passes the filter. Likewise, if the duration check
fails, the filter result does not matter. So I was wondering whether
the decision logic could be arranged to avoid query ID lookups in
those cases.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  17. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-07-02T04:27:30Z

    čt 2. 7. 2026 v 0:09 odesílatel Zsolt Parragi <zsolt.parragi@percona.com>
napsal:

> +      <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
>
> I would still improve this part a bit, ID is singular at both places
> but it should be plural IDs, and there's a double space at "this
> parameter".
>

fixed


>
> Other than these I have no more comments, the patch looks good to me.
>

thank you

Regards

Pavel

  18. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-07-02T04:50:21Z

    čt 2. 7. 2026 v 5:01 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com>
napsal:

> Hello,
>
> At Wed, 1 Jul 2026 21:51:30 +0200, Pavel Stehule <pavel.stehule@gmail.com>
> wrote in
> > > 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.
>
> I think there may be a misunderstanding.
>
> I was not suggesting that one GUC should force another GUC. My point
> was about when auto_explain calls EnableQueryId(), not about changing
> the value of compute_query_id.
>
> Instead of requesting query ID calculation unconditionally when the
> module is loaded, I was wondering whether EnableQueryId() could be
> called from assign_log_queryids(), so that auto_explain only requests
> query ID calculation when it is actually going to use it.
>

Unfortunately EnableQueryId is not reversible.

So when users force EnableQueryId by non-empty log_queryids, then can be
confused
when trying to set empty log_queryid, but the queryid will be computed
until the end of session.

If I can switch queryid computing to between on, off, then this can be good
idea, but becase
I can only enable computing, then I don't like it too much.


> > 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.
>
> I think that is a slightly different issue.
>
> I was not suggesting that queryids_filter itself should force query ID
> calculation, nor that we should emit a warning when query ID is not
> available.
>
> My point was that, if log_queryids is the option that makes
> auto_explain use the query ID, then auto_explain could call
> EnableQueryId() when that option is enabled. In that case, the
> situation where queryids_filter is set but no query ID is calculated
> would not be introduced by this change; if log_queryids is off,
> auto_explain is not going to use the query ID anyway.
>



>
> > > -               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.
>
> Yes, that behavior makes sense. I was not suggesting changing the
> meaning of an empty queryids_filter.
>
> My comment was about the ordering of the checks in the proposed patch.
> If queryids_filter is empty, no query ID lookup is needed to decide
> whether the query passes the filter. Likewise, if the duration check
> fails, the filter result does not matter. So I was wondering whether
> the decision logic could be arranged to avoid query ID lookups in
> those cases.
>

do you think some like

if (!queryid_filter || log_filter) && (msec >=
auto_explain_log_min_duration))

??


> Regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

  19. Re: proposal - queryid can be used as filter for auto_explain

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2026-07-02T07:04:03Z

    At Thu, 2 Jul 2026 06:50:21 +0200, Pavel Stehule <pavel.stehule@gmail.com> wrote in 
> Unfortunately EnableQueryId is not reversible.
> 
> So when users force EnableQueryId by non-empty log_queryids, then can be
> confused
> when trying to set empty log_queryid, but the queryid will be computed
> until the end of session.
> 
> If I can switch queryid computing to between on, off, then this can be good
> idea, but becase
> I can only enable computing, then I don't like it too much.

There is also the opposite way of looking at it.

With the current behavior, simply loading auto_explain causes query ID
calculation to be requested, even if the session never intends to use
query IDs. Disabling that requires setting compute_query_id to off,
which also affects other modules. That seems less natural to me than
keeping query ID calculation enabled after log_queryids has been
enabled once in the session.

To me, requesting query ID calculation only when auto_explain is
actually configured to use it better matches the user's expectation,
even if the request itself is not reversible.

Of course, if others still think that enabling query ID calculation at
module load time is the better choice, then I don't have any strong
objection.

> > My comment was about the ordering of the checks in the proposed patch.
> > If queryids_filter is empty, no query ID lookup is needed to decide
> > whether the query passes the filter. Likewise, if the duration check
> > fails, the filter result does not matter. So I was wondering whether
> > the decision logic could be arranged to avoid query ID lookups in
> > those cases.
> >
> 
> do you think some like
> 
> if (!queryid_filter || log_filter) && (msec >=
> auto_explain_log_min_duration))
> 
> ??

Not quite. My point was that if auto_explain_log_min_duration already
rejects the query, then any query ID lookup is unnecessary.

So I was thinking more along these lines:

if (msec >= auto_explain_log_min_duration)
{
    if (queryids_filter_is_empty || queryid_matches_filter())
        log_plan();
}

The important part is that queryid_matches_filter() should only be
called after the duration check has passed and only when the filter is
not empty.

The helper function above is just for illustration. I don't mean to
suggest introducing such a function; I'm perfectly fine with keeping the
decision logic open-coded if that fits the existing code better.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  20. Re: proposal - queryid can be used as filter for auto_explain

    Pavel Stehule <pavel.stehule@gmail.com> — 2026-07-02T20:04:40Z

    Hi

čt 2. 7. 2026 v 9:04 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com>
napsal:

> At Thu, 2 Jul 2026 06:50:21 +0200, Pavel Stehule <pavel.stehule@gmail.com>
> wrote in
> > Unfortunately EnableQueryId is not reversible.
> >
> > So when users force EnableQueryId by non-empty log_queryids, then can be
> > confused
> > when trying to set empty log_queryid, but the queryid will be computed
> > until the end of session.
> >
> > If I can switch queryid computing to between on, off, then this can be
> good
> > idea, but becase
> > I can only enable computing, then I don't like it too much.
>
> There is also the opposite way of looking at it.
>
> With the current behavior, simply loading auto_explain causes query ID
> calculation to be requested, even if the session never intends to use
> query IDs. Disabling that requires setting compute_query_id to off,
> which also affects other modules. That seems less natural to me than
> keeping query ID calculation enabled after log_queryids has been
> enabled once in the session.
>
> To me, requesting query ID calculation only when auto_explain is
> actually configured to use it better matches the user's expectation,
> even if the request itself is not reversible.
>
> Of course, if others still think that enabling query ID calculation at
> module load time is the better choice, then I don't have any strong
> objection.
>

I understand - and the main problem is fact so query ID calculation doesn't
allow more accurate management.

I think so any way is legitime - just depends what is preferred -
robustness or effectivity

There is not any precedent in core - for pg_stat_statements and
pg_stash_advice the queryid is required always.

So I see a few possibilities:

1. EnableQueryId on start - it is the generic most simple solution - in my
experience - it is only safe guard for cases, where auto_explain is used
without pg_stat_statements (which is not too common). Negative impact -
queryid will be calculated although it can be useless (until somebody
explicitly disables it). But it is only in cases where queryid is not
required by another module.

2. Delayed call EnableQueryId - called in GUC assignment - it should work
too. I intuitively dislike it, because it is only a one way switch.

3. Don't call EnableQueryId in auto_explain - we can document - if
log_queryids is used, then compute_queryid should be ON.

4. Introduction of a new API that allows accurate queryid computing
management.  Any module can signalize dynamically if a queryid is required.
This needs a deeper discussion.

Please, everybody, what do you prefer?


>
> > > My comment was about the ordering of the checks in the proposed patch.
> > > If queryids_filter is empty, no query ID lookup is needed to decide
> > > whether the query passes the filter. Likewise, if the duration check
> > > fails, the filter result does not matter. So I was wondering whether
> > > the decision logic could be arranged to avoid query ID lookups in
> > > those cases.
> > >
> >
> > do you think some like
> >
> > if (!queryid_filter || log_filter) && (msec >=
> > auto_explain_log_min_duration))
> >
> > ??
>
> Not quite. My point was that if auto_explain_log_min_duration already
> rejects the query, then any query ID lookup is unnecessary.
>
> So I was thinking more along these lines:
>
> if (msec >= auto_explain_log_min_duration)
> {
>     if (queryids_filter_is_empty || queryid_matches_filter())
>         log_plan();
> }
>
> The important part is that queryid_matches_filter() should only be
> called after the duration check has passed and only when the filter is
> not empty.
>
> The helper function above is just for illustration. I don't mean to
> suggest introducing such a function; I'm perfectly fine with keeping the
> decision logic open-coded if that fits the existing code better.
>

understand - changed

Regards

Pavel


>
> Regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>