Re: compute_query_id and pg_stat_statements - Mailing list pgsql-hackers

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