| From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
|---|---|
| To: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: proposal - queryid can be used as filter for auto_explain |
| Date: | 2026-06-19 06:05:52 |
| Message-ID: | CAFj8pRC-Pskt-xDOewehyAkijSKPbaCsh9SfhKrVdeNc-Uqd4Q@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi
čt 18. 6. 2026 v 23:01 odesílatel Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)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
| Attachment | Content-Type | Size |
|---|---|---|
| v20260619-3-0001-auto_explain.log_queryids.patch | text/x-patch | 10.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Jim Jones | 2026-06-19 06:07:38 | Re: XMLSerialize: version and explicit XML declaration |
| Previous Message | Tristan Partin | 2026-06-19 04:49:18 | Re: PG20 Minimum Dependency Thread |