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.