Re: Feature improvement for pg_stat_statements

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,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION