Re: [PATCH] Add features to pg_stat_statements

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

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?

Regards,
Seino Yuki