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

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
argumentsuserid, 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