Re: Feature improvement for pg_stat_statements

2020-12-01 01:04 に Fujii Masao さんは書きました:
> On 2020/11/30 23:05, Seino Yuki wrote:
>> 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.
> 
> I was thinking that GetCurrentTimestamp() should be called before
> pgss->lock lwlock is taken, only when all three arguments userid, dbid
> and queryid are zero. But on second thought, we should call
> GetCurrentTimestamp() and reset the stats, after the following codes?
> 
>     /* All entries are removed? */
>     if (num_entries != num_remove)
>         goto release_lock;
> 
> That is, IMO that even when pg_stat_statements_reset() with non-zero
> arguments is executed, if it removes all the entries, we should reset
> the stats. Thought?
> 
> Regards,

+1.Fixed the patch.
We tested various reset patterns and they seemed to be fine.

Regards.