Re: compute_query_id and pg_stat_statements

On 2021/05/13 13:03, Julien Rouhaud wrote:
> On Wed, May 12, 2021 at 11:33:32PM -0400, Bruce Momjian wrote:
>> On Thu, May 13, 2021 at 11:16:13AM +0800, Julien Rouhaud wrote:
>>> On Wed, May 12, 2021 at 11:06:52PM -0400, Bruce Momjian wrote:
>>>> On Thu, May 13, 2021 at 09:57:00AM +0800, Julien Rouhaud wrote:
>>>>
>>>>> source? What if you have for instance pg_stat_statements, pg_stat_kcache,
>>>>> pg_store_plans and pg_wait_sampling installed? All those extensions need a
>>>>> query_id (or at least benefit from it for pg_wait_sampling), is there any value
>>>>> to give a full list of all the modules that would enable compute_query_id?
>>>>
>>>> Well, we don't have any other cases where the source of the change is
>>>> this indeterminate, so I don't really have a suggestion. I think this
>>>> does highlight another case where 'auto' just isn't a good fit for our
>>>> API.
>>>
>>> It may depends on your next question
>>>
>>>>> I'm assuming that anyone wanting to install any of those extensions (or any
>>>>> similar one) is fully aware that they aggregate metrics based on at least a
>>>>> query_id. If they don't, well they probably never read any documentation since
>>>>> postgres 9.2 which introduced query normalization, and I doubt that they will
>>>>> be interested in having access to the information anyway.
>>>>
>>>> My point is that we are changing a setting from an extension, and if you
>>>> look in pg_setting, it will say "default"?
>>>
>>> No, it will say "on". What the patch I sent implements is:
>>
>> I was asking what pg_settings.source will say:
>>
>> SELECT name, soource FROM pg_settings;
>
> Oh sorry. Here's the full output before and after a dynamic call to
> queryIsWanted():
>
> name | compute_query_id
> setting | auto
> unit | <NULL>
> category | Statistics / Monitoring
> short_desc | Compute query identifiers.
> extra_desc | <NULL>
> context | superuser
> vartype | enum
> source | default
> min_val | <NULL>
> max_val | <NULL>
> enumvals | {auto,on,off}
> boot_val | auto
> reset_val | auto
> sourcefile | <NULL>
> sourceline | <NULL>
> pending_restart | f
>
> =# LOAD 'pg_qualstats'; /* will call queryIsWanted() */
> WARNING: 01000: Without shared_preload_libraries, only current backend stats will be available.
> LOAD
>
> name | compute_query_id
> setting | on
> unit | <NULL>
> category | Statistics / Monitoring
> short_desc | Compute query identifiers.
> extra_desc | <NULL>
> context | superuser
> vartype | enum
> source | default
> min_val | <NULL>
> max_val | <NULL>
> enumvals | {auto,on,off}
> boot_val | auto
> reset_val | auto
> sourcefile | <NULL>
> sourceline | <NULL>
> pending_restart | f
>
> So yes, it says "default" (and it's the same if the change is done during
> postmaster init). It can be fixed if that's the only issue.

I like leaving compute_query_id=auto instead of overwriting it to "on",
even when queryIsWanted() is called, as I told upthread. Then we can decide
that query id computation is necessary when compute_query_id=auto and
the special flag is enabled by queryIsWanted(). Of course as you and Magnus
discussed upthread, the issue in EXEC_BACKEND case should be fixed,
maybe by using save_backend_variables() or something, though.

If we do this, compute_query_id=auto seems to be similar to huge_pages=try.
When huge_pages=try, whether huge pages is actually used is defined depending
outside PostgreSQL (i.e, OS setting in this case). Neither pg_settings.setting nor
souce are not changed in that case.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION