Re: Feature improvement for pg_stat_statements
От | Fujii Masao |
---|---|
Тема | Re: Feature improvement for pg_stat_statements |
Дата | |
Msg-id | a426afdb-8f22-6793-2cdb-eb15ca4a63ef@oss.nttdata.com обсуждение исходный текст |
Ответ на | Re: Feature improvement for pg_stat_statements (Seino Yuki <seinoyu@oss.nttdata.com>) |
Список | pgsql-hackers |
On 2020/12/17 22:59, Seino Yuki wrote: > 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. So I pushed the patch. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Bharath RupireddyДата:
Сообщение: Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit
Следующее
От: Bharath RupireddyДата:
Сообщение: Re: New Table Access Methods for Multi and Single Inserts