Thread: Feature improvement for pg_stat_statements
Hello. I’d like to improve pg_stat_statements so that it can report the timestamp when the stats are reset. Currently it’s difficult to check that reset timestamp. But this information is useful, for example, when calculating the SQL executions per second by SELECT calls / (now() - reset_timestamp) FROM pg_stat_statements. I’m thinking of adding adding a function called pg_stat_statements_reset_time() that returns the last timestamp when executed pg_stat_statements_reset(). pg_stat_statements can reset each SQL statement. We can record each sql reset timing timestamp but I don’t feel the need. This time, I’m thinking of updating the reset timestamp only when all stats were reset. what do you think ? Regards. Naoki Nakamichi
That'd be useful in my book. My scripts just have a hard coded timestamp I replace when I call reset so those calculations work, but it would be much preferred to have that data available by a built in function.
-Adam
Hi, On 2020-09-18 17:55:12 +0900, btnakamichin wrote: > I’m thinking of adding adding a function called > pg_stat_statements_reset_time() that returns the last timestamp when > executed pg_stat_statements_reset(). pg_stat_statements can reset each SQL > statement. We can record each sql reset timing timestamp but I don’t feel > the need. This time, I’m thinking of updating the reset timestamp only when > all stats were reset. What exactly do you mean by "can reset each SQL statement"? I don't really see what alternative you're analyzing? It does make sense to me to have a function returning the time of the last reset. Greetings, Andres Freund
2020-09-22 04:58 に Andres Freund さんは書きました: > Hi, > > On 2020-09-18 17:55:12 +0900, btnakamichin wrote: >> I’m thinking of adding adding a function called >> pg_stat_statements_reset_time() that returns the last timestamp when >> executed pg_stat_statements_reset(). pg_stat_statements can reset each >> SQL >> statement. We can record each sql reset timing timestamp but I don’t >> feel >> the need. This time, I’m thinking of updating the reset timestamp only >> when >> all stats were reset. > > What exactly do you mean by "can reset each SQL statement"? I don't > really see what alternative you're analyzing? > > It does make sense to me to have a function returning the time of the > last reset. > > Greetings, > > Andres Freund I’m Sorry, I forgot to include pgsql_hackers in the cc, so I resend it and attach the patch. Thank you, I appreciate your comment. I am unfamiliar with this function. I’m sorry if my interpretation is mistaken. > What exactly do you mean by "can reset each SQL statement"? I don't > really see what alternative you're analyzing? pg_stat_statements_reset discards statistics gathered so far by pg_stat_statements corresponding to the specified userid, dbid and queryid. If no parameter is specified or all the specified parameters are 0(invalid), it will discard all statistics. I think that it is important to record timestamp discarding all statistics so I’d like to add a function for only all stats were reset. The following is an example of this feature. postgres=# select * from pg_stat_statements_reset_time(); pg_stat_statements_reset_time ------------------------------- 2020-09-24 13:36:44.87005+09 (1 row) Best regards, Naoki Nakamichi
Attachment
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, failed
Spec compliant: tested, passed
Documentation: tested, failed
The patch applies cleanly and looks fine to me.I'm going to list a few points that I think need to be fixed.
1.There are unnecessary difference lines 89 in "pg_stat_statements.c".
2.It is confusing that the initial value of reset_time is the current date and time, so why not set it to null?
3.How about pg_stat_statements_reset_time creates a view?
4.How about counting the number of resets?
5."pgstatstatstatements.sgml" would need to include the function name in the following section.
- these statistics, the module provides a view, <structname>pg_stat_statements</structname>,
- and the utility functions <function>pg_stat_statements_reset</function> and
- <function>pg_stat_statements</function>. These are not available globally but
- can be enabled for a specific database with
+ these statistics, the module provides views, <structname>pg_stat_statements</structname>
+ and <structname>pg_stat_statements_ctl</structname>,
+ and the utility functions <function>pg_stat_statements_reset</function>,
+ <function>pg_stat_statements</function>, and <function>pg_stat_statements_reset_time</function>.
+ These are not available globally but can be enabled for a specific database with
It would be nice to be able to keep the timing of resetting the userid, dbid and queryid as well, but I think the
statusquo is appropriate for management in memory.
The new status of this patch is: Waiting on Author
At Fri, 16 Oct 2020 10:47:50 +0000, Yuki Seino <seinoyu@oss.nttdata.com> wrote in > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, failed > Spec compliant: tested, passed > Documentation: tested, failed > > The patch applies cleanly and looks fine to me.I'm going to list a few points that I think need to be fixed. > > 1.There are unnecessary difference lines 89 in "pg_stat_statements.c". > 2.It is confusing that the initial value of reset_time is the current date and time, so why not set it to null? > 3.How about pg_stat_statements_reset_time creates a view? > 4.How about counting the number of resets? > 5."pgstatstatstatements.sgml" would need to include the function name in the following section. > - these statistics, the module provides a view, <structname>pg_stat_statements</structname>, > - and the utility functions <function>pg_stat_statements_reset</function> and > - <function>pg_stat_statements</function>. These are not available globally but > - can be enabled for a specific database with > + these statistics, the module provides views, <structname>pg_stat_statements</structname> > + and <structname>pg_stat_statements_ctl</structname>, > + and the utility functions <function>pg_stat_statements_reset</function>, > + <function>pg_stat_statements</function>, and <function>pg_stat_statements_reset_time</function>. > + These are not available globally but can be enabled for a specific database with > > It would be nice to be able to keep the timing of resetting the userid, dbid and queryid as well, but I think the statusquo is appropriate for management in memory. > > The new status of this patch is: Waiting on Author + SpinLockAcquire(&pgss->mutex); You might noticed, but there a purpose of using the following idiom. Without that, compiler might optimize out the comparison assuming *pgss won't change. > volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; \ > SpinLockAcquire(&s->mutex); \ + SpinLockAcquire(&pgss->mutex); + pgss->reset_time = GetCurrentTimestamp(); We should avoid (possiblly) time-cosuming thing like GetCurrentTimestamp(); + this function shows last reset time only when <function>pg_stat_statements_reset(0,0,0)</function> is called. This reads as "この関数はpg_statstatement_reset(0,0,0) が呼び出された ときにだけ最終リセット時刻を表示します。", which I think is different from what is intentended. And the wording is not alike with the documentation for similar functions. https://www.postgresql.org/docs/13/functions-datetime.html > current_timestamp → timestamp with time zone > Current date and time (start of current transaction); see Section 9.9.4 https://www.postgresql.org/docs/13/monitoring-stats.html pg_stat_archiver view > stats_reset timestamp with time zone > Time at which these statistics were last reset So something like the following? Time at which pg_stat_statements_reset(0,0,0) was last called. or Time at which statistics are last discarded by calling pg_stat_statements_reset(0,0,0). regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Thank you for pointing that out. I'll post a fixed patch. > + SpinLockAcquire(&pgss->mutex); > > You might noticed, but there a purpose of using the following > idiom. Without that, compiler might optimize out the comparison > assuming *pgss won't change. > >> volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; \ >> SpinLockAcquire(&s->mutex); \ > > + SpinLockAcquire(&pgss->mutex); > + pgss->reset_time = GetCurrentTimestamp(); Fix the use of this idiom when modifying *pgss. > We should avoid (possiblly) time-cosuming thing like > GetCurrentTimestamp(); I understood your point to be "It's better not to execute GetCurrentTimestamp() while spinlock is running. Fix to store the result of GetCurrentTimestamp() in a local variable before running spinlock. This value is stored in reset_time. > + this function shows last reset time only when > <function>pg_stat_statements_reset(0,0,0)</function> is called. > > This reads as "この関数はpg_statstatement_reset(0,0,0) が呼び出された > ときにだけ最終リセット時刻を表示します。", which I think is different > from what is intentended. > > And the wording is not alike with the documentation for similar > functions. > > https://www.postgresql.org/docs/13/functions-datetime.html >> current_timestamp → timestamp with time zone >> Current date and time (start of current transaction); see Section >> 9.9.4 > > https://www.postgresql.org/docs/13/monitoring-stats.html > pg_stat_archiver view >> stats_reset timestamp with time zone >> Time at which these statistics were last reset > > So something like the following? > > Time at which pg_stat_statements_reset(0,0,0) was last called. > > or > > Time at which statistics are last discarded by calling > pg_stat_statements_reset(0,0,0). I have made the following corrections. this function shows last reset time only when <function>pg_stat_statements_reset(0,0,0)</function> is called. →this function shows the time at which <function>pg_stat_statements_reset(0,0,0)</function> was last called. <function>pg_stat_statements_reset_time(void) returns time stamp with time zone</function> →<function>pg_stat_statements_reset_time(void) returns timestamp with time zone</function> Regards.
Attachment
Due to similar fixed, we have also merged the patches discussed in the following thread. https://commitfest.postgresql.org/30/2736/ The patch is posted on the 2736 side of the thread. Regards.
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/ > The patch is posted on the 2736 side of the thread. > > Regards. The following patches have been committed and we have created a compliant patch for them. https://commitfest.postgresql.org/30/2736/ Please confirm. Regards.
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/ >> 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/ Please confirm. Regards.
Attachment
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/ >>> 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/ > > 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, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
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/ >>>> 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/ >> >> 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. I learned a lot, especially since I didn't know about MemSet(). Regards.
Attachment
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/ >>>>> 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/ >>> >>> 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, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
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/ >>>>>> 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/ >>>> >>>> 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. Regards.
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/ >>>>>>> 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/ >>>>> >>>>> 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 argumentsuserid, 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, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
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/ >>>>>>>> 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/ >>>>>> >>>>>> 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. Regards.
Attachment
Hi,
On Dec 9, 2020, at 6:37 PM, Seino Yuki <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 さんは書きました:I was thinking that GetCurrentTimestamp() should be called beforeOn 2020/11/30 12:08, Seino Yuki wrote:Thanks for the new comment.2020-11-27 22:39 に Fujii Masao さんは書きました:Thanks for updating the patch! Here are another review comments.On 2020/11/27 21:39, Seino Yuki wrote:Thanks for the review!2020-11-27 21:37 に Seino Yuki さんは書きました:Thanks for updating the patch! Here are review comments from me.2020-11-16 12:28 に Seino Yuki さんは書きました:I forgot the attachment and will resend it.Due to similar fixed, we have also merged the patches discussed in the
following thread.
https://commitfest.postgresql.org/30/2736/
The patch is posted on the 2736 side of the thread.
Regards.
The following patches have been committed and we have created a
compliant patch for them.
https://commitfest.postgresql.org/30/2736/
Please confirm.
+ 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,
Fixed the patch.
+ <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,
I got the following pointers earlier.+ reset_ts = GetCurrentTimestamp();GetCurrentTimestamp() needs to be called only when all the entriesWhich do you think is better? I think the new pointing out is better,
are removed. But it's called even in other cases.
because entry_reset is not likely to be called often.
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);
--
Best regards
ChengDu WenWu Information Technology Co.,Ltd.
Japin Li
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
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.
Attachment
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
Attachment
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.
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