Re: [PATCH] Add features to pg_stat_statements

On 2020/10/28 17:31, seinoyu wrote:
> 2020-10-22 01:31 に Fujii Masao さんは書きました:
>> On 2020/10/12 21:18, Yuki Seino wrote:
>>> The following review has been posted through the commitfest application:
>>> make installcheck-world:  tested, passed
>>> Implements feature:       tested, passed
>>> Spec compliant:           tested, passed
>>> Documentation:            tested, passed
>>>
>>> The patch applies cleanly and looks fine to me. It's a small detail, However wouldn't it be better if the following
changeswere made.
 
>>>
>>> 1.There are unnecessary difference lines 707 and 863 in "pg_stat_statements.c".
>>> 2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in "pg_stat_statements.c".
>>> 3."update_ctl" and "reset_ctl" are generic and illegible name.You might want to rename it something like
"pgss_ctl_update".
>>> 4.To improve the readability of the source, why not change the function declaration option in
"pg_stat_statements_ctl"from
 
>>> "AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".
>>>
>>> The new status of this patch is: Waiting on Author
>>
>> Here are other comments from me.
>>
>> -DATA = pg_stat_statements--1.4.sql \
>> +DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\
>>
>> One space character needs to be added just before the character "\".
>>
>> +--- Define pg_stat_statements_ctl
>>
>> ISTM that this is not good name.
>> What about pg_stat_statements_info, _stats, _activity or something?
>>
>> +    OUT dealloc integer,
>>
>> The type of "dealloc" should be bigint, instead?
>>
>> +    OUT last_dealloc TIMESTAMP WITH TIME ZONE
>>
>> Is this information really useful?
>> If there is no valid use case for this, I'd like to drop it.
>> Thought?
>>
>> +LANGUAGE  C STRICT VOLATILE PARALLEL SAFE;
>>
>> There are two space characters just after "LANGUAGE".
>> One space character should be removed from there.
>>
>> +CREATE VIEW pg_stat_statements_ctl AS
>> +    SELECT * FROM pg_stat_statements_ctl();
>>
>> If we rename the function, this view name also should be changed.
>>
>> +GRANT SELECT ON pg_stat_statements TO PUBLIC;
>>
>> "pg_stat_statements" should be "pg_stat_statement_xxx"?
>>
>> Regards,
> 
> 
> Thanks for the comment, Fujii-san.
> 
> I will post the patch again in the future to reflect your and my points.

Thanks!

> 
> However, let me confirm the following.
>> Is this information really useful?
>> If there is no valid use case for this, I'd like to drop it.
>> Thought?
> 
> I thought it would be easy for users to see at a glance that if there is a case I assumed,
> if the last modified date and time is old, there is no need to adjust at all, and if the
> last modified date and time is recent, it would be easy for users to understand that the
> parameters need to be adjusted.
> What do you think?

Even without the last deallocation timestamp, you can presume
when the deallocation of entries happened by periodically monitoring
the total number of deallocations and seeing those history. Or IMO
it's better to log whenever the deallocation happens as proposed upthread.
Then you can easily check the history of occurrences of deallocations
from the log file.

Regards,

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