Re: Feature improvement for pg_stat_statements
От | Seino Yuki |
---|---|
Тема | Re: Feature improvement for pg_stat_statements |
Дата | |
Msg-id | b8ae9fed8b53334f77dafd851a895892@oss.nttdata.com обсуждение исходный текст |
Ответ на | Re: Feature improvement for pg_stat_statements (Fujii Masao <masao.fujii@oss.nttdata.com>) |
Ответы |
Re: Feature improvement for pg_stat_statements
(Fujii Masao <masao.fujii@oss.nttdata.com>)
|
Список | pgsql-hackers |
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.
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Ashutosh BapatДата:
Сообщение: Misleading comment in prologue of ReorderBufferQueueMessage