Re: Feature improvement for pg_stat_statements - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: Feature improvement for pg_stat_statements |
Date | |
Msg-id | 808d645e-b20c-c02c-0c39-224ba80426ca@oss.nttdata.com Whole thread Raw |
In response to | Re: Feature improvement for pg_stat_statements (Li Japin <japinli@hotmail.com>) |
Responses |
Re: Feature improvement for pg_stat_statements
|
List | pgsql-hackers |
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, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
pgsql-hackers by date: