Re: Feature improvement for pg_stat_statements
От | Fujii Masao |
---|---|
Тема | Re: Feature improvement for pg_stat_statements |
Дата | |
Msg-id | 8132187e-a24d-a192-ac26-1e4c693c5218@oss.nttdata.com обсуждение исходный текст |
Ответ на | Re: Feature improvement for pg_stat_statements (Seino Yuki <seinoyu@oss.nttdata.com>) |
Ответы |
Re: Feature improvement for pg_stat_statements
(Seino Yuki <seinoyu@oss.nttdata.com>)
|
Список | pgsql-hackers |
On 2020/12/14 18:17, Seino Yuki wrote: > 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. Thanks for updating the patch! + reset_ts = GetCurrentTimestamp(); + /* Reset global statistics for pg_stat_statements */ + { + volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; + + SpinLockAcquire(&s->mutex); + s->stats.stats_reset = reset_ts; /* reset execution time */ + SpinLockRelease(&s->mutex); This code makes me think that "dealloc" field also should be reset at the same time together, i.e., whenever all entriesare removed. Otherwise, even when pg_stat_statements_reset() with non-zero arguments discards all statistics, "dealloc"field in pg_stat_statements_info is not reset. Only pg_stat_statements_reset(0, 0, 0) resets "dealloc" field. Thatseems confusing. Thought? I updated the patch so that "dealloc" field is also reset whenever all entries are removed. Attached is the updated versionof the patch. + result_tuple = heap_form_tuple(tupdesc, values, nulls); + return HeapTupleGetDatum(result_tuple); Isn't it better to use PG_RETURN_DATUM() here, instead? I applied this change to the patch. I also applied some cosmetic changes to the patch. Could you review this version? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Heikki LinnakangasДата:
Сообщение: Re: Feature Proposal: Add ssltermination parameter for SNI-based LoadBalancing