Re: Feature improvement for pg_stat_statements

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.