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(at)oss(dot)nttdata(dot)com
>>> <mailto:seinoyu(at)oss(dot)nttdata(dot)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.