Re: Feature improvement for pg_stat_statements
От | Seino Yuki |
---|---|
Тема | Re: Feature improvement for pg_stat_statements |
Дата | |
Msg-id | 91e2d76693ef33e0b429bc947f70bbbe@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-14 23:01 に Fujii Masao さんは書きました: > 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 entries are 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. That seems confusing. Thought? > > I updated the patch so that "dealloc" field is also reset whenever all > entries are removed. Attached is the updated version of 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, Thank you for providing the patch. I have checked the operation. There was no problem. Regards.
В списке pgsql-hackers по дате отправления: