Feature improvement for pg_stat_statements

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

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

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

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.

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.

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.

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

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.

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

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/&gt;
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/&gt;
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/&gt;
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/&gt;
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.

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/&gt;
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/&gt;
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