Horton

Thread

Commits

  1. pg_stat_statements: Track time at which all statistics were last reset.

  1. Feature improvement for pg_stat_statements

    btnakamichin <btnakamichin@oss.nttdata.com> — 2020-09-18T08:55:12Z

    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

  2. Re: Feature improvement for pg_stat_statements

    Adam Brusselback <adambrusselback@gmail.com> — 2020-09-18T14:50:34Z

    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

  3. Re: Feature improvement for pg_stat_statements

    Andres Freund <andres@anarazel.de> — 2020-09-21T19:58:39Z

    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

  4. Re: Feature improvement for pg_stat_statements

    btnakamichin <btnakamichin@oss.nttdata.com> — 2020-09-24T04:54:27Z

    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

  5. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-10-16T10:47:50Z

    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 status quo is appropriate for management in memory.

The new status of this patch is: Waiting on Author

  6. Re: Feature improvement for pg_stat_statements

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-10-16T12:33:50Z

    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 status quo 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

  7. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-09T11:09:29Z

    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.

  8. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-16T03:28:38Z

    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.

  9. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-27T12:37:23Z

    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.

  10. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-27T12:39:00Z

    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.

  11. Re: Feature improvement for pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-27T13:39:27Z

    
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

  12. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-30T03:08:48Z

    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.

  13. Re: Feature improvement for pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-30T06:02:03Z

    
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

  14. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-30T14:05:25Z

    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.

  15. Re: Feature improvement for pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-30T16:04:18Z

    
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,


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  16. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-12-09T10:37:57Z

    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.

  17. Re: Feature improvement for pg_stat_statements

    Japin Li <japinli@hotmail.com> — 2020-12-09T11:42:45Z

    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/
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.


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

  18. Re: Feature improvement for pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-12-09T12:14:50Z

    
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

  19. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-12-14T09:17:58Z

    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.

  20. Re: Feature improvement for pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-12-14T14:01:52Z

    
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,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  21. Re: Feature improvement for pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-12-17T13:59:20Z

    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.

  22. Re: Feature improvement for pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-12-18T01:52:11Z

    
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