Re: Feature improvement for pg_stat_statements - Mailing list pgsql-hackers

2020-11-30 15:02 に Fujii Masao さんは書きました:
> On 2020/11/30 12:08, Seino Yuki wrote:
>> 2020-11-27 22:39 に Fujii Masao さんは書きました:
>>> On 2020/11/27 21:39, Seino Yuki wrote:
>>>> 2020-11-27 21:37 に Seino Yuki さんは書きました:
>>>>> 2020-11-16 12:28 に Seino Yuki さんは書きました:
>>>>>> Due to similar fixed, we have also merged the patches discussed in 
>>>>>> the
>>>>>> following thread.
>>>>>> https://commitfest.postgresql.org/30/2736/
>>>>>> The patch is posted on the 2736 side of the thread.
>>>>>> 
>>>>>> Regards.
>>>>> 
>>>> 
>>>> I forgot the attachment and will resend it.
>>>> 
>>>> The following patches have been committed and we have created a
>>>> compliant patch for them.
>>>> https://commitfest.postgresql.org/30/2736/
>>>> 
>>>> Please confirm.
>>> 
>>> Thanks for updating the patch! Here are review comments from me.
>>> 
>>> +    OUT reset_exec_time TIMESTAMP WITH TIME ZONE
>>> 
>>> I prefer "stats_reset" as the name of this column for the sake of
>>> consistency with the similar column in other stats views like
>>> pg_stat_database.
>>> 
>>> Isn't it better to use lower cases for "TIMESTAMP WITH TIME ZONE"
>>> because other DDLs in pg_stat_statements do that for the declaraion
>>> of data type?
>>> 
>>> @@ -565,6 +568,8 @@ pgss_shmem_startup(void)
>>>          pgss->n_writers = 0;
>>>          pgss->gc_count = 0;
>>>          pgss->stats.dealloc = 0;
>>> +        pgss->stats.reset_exec_time = 0;
>>> +        pgss->stats.reset_exec_time_isnull = true;
>>> 
>>> The reset time should be initialized with GetCurrentTimestamp() 
>>> instead
>>> of 0? If so, I think that we can completely get rid of 
>>> reset_exec_time_isnull.
>>> 
>>> +    memset(nulls, 0, sizeof(nulls));
>>> 
>>> If some entries in "values[]" may not be set, "values[]" also needs 
>>> to
>>> be initialized with 0.
>>> 
>>> MemSet() should be used, instead?
>>> 
>>> +    /* Read dealloc */
>>> +    values[0] = stats.dealloc;
>>> 
>>> Int64GetDatum() should be used here?
>>> 
>>> +    reset_ts = GetCurrentTimestamp();
>>> 
>>> GetCurrentTimestamp() needs to be called only when all the entries
>>> are removed. But it's called even in other cases.
>>> 
>>> Regards,
>> 
>> Thanks for the review!
>> Fixed the patch.
> 
> Thanks for updating the patch! Here are another review comments.
> 
> +       <structfield>reset_exec_time</structfield> <type>timestamp
> with time zone</type>
> 
> You forgot to update the column name in the doc?
> 
> +        Shows the time at which
> <function>pg_stat_statements_reset(0,0,0)</function> was last called.
> 
> What about updating this to something like "Time at which all 
> statistics
> in the pg_stat_statements view were last reset." for the sale of
> onsistency with the description about stats_reset column in other
> tats views?
> 
> +    /* Read stats_reset */
> +    values[1] = stats.stats_reset;
> 
> TimestampTzGetDatum() seems necessary.
> 
> +        reset_ts = GetCurrentTimestamp();
>          /* Reset global statistics for pg_stat_statements */
> 
> Isn't it better to call GetCurrentTimestamp() before taking
> an exclusive lwlock, in entry_reset()?
> 
> Regards,

Thanks for the new comment.

I got the following pointers earlier.

> +    reset_ts = GetCurrentTimestamp();

> GetCurrentTimestamp() needs to be called only when all the entries
> are removed. But it's called even in other cases.

Which do you think is better? I think the new pointing out is better,
because entry_reset is not likely to be called often.

Regards.