Re: Feature improvement for pg_stat_statements

2020-12-09 21:14 に Fujii Masao さんは書きました:
> On 2020/12/09 20:42, Li Japin wrote:
>> Hi,
>> 
>>> On Dec 9, 2020, at 6:37 PM, Seino Yuki <seinoyu@oss.nttdata.com 
>>> <mailto:seinoyu@oss.nttdata.com>> wrote:
>>> 
>>> 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/ 
>>>>>>>>>>> <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/ 
>>>>>>>>> <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.
>>> 
>> 
>> Since BlessTupleDesc() is for SRFs according to the comments, I think, 
>> we can
>> remove it here.  Correct?
>> 
>> +       if (get_call_result_type(fcinfo, NULL, &tupdesc) != 
>> TYPEFUNC_COMPOSITE)
>> +               elog(ERROR, "return type must be a row type");
>> +
>> +       tupdesc = BlessTupleDesc(tupdesc);
> 
> Here are other comments from me:
> 
> The patch seems to contain whitespace errors.
> You can see them by "git diff --check".
> 
> +      <para>
> +        Time at which all statistics in the pg_stat_statements view
> were last reset.
> +      </para></entry>
> 
> "pg_stat_statements" in the above should be enclosed with
> <structname> and </structname>.
> 
> Regards,

Thank you for your comments, Mr. Fujii and Mr. Li.
I've posted a patch reflecting your comments.
Please check it out.