Feature improvement for pg_stat_statements

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Feature improvement for pg_stat_statements

btnakamichin
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


Reply | Threaded
Open this post in threaded view
|

Re: Feature improvement for pg_stat_statements

Adam Brusselback
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
Reply | Threaded
Open this post in threaded view
|

Re: Feature improvement for pg_stat_statements

Andres Freund
In reply to this post by btnakamichin
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


Reply | Threaded
Open this post in threaded view
|

Re: Feature improvement for pg_stat_statements

btnakamichin
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

pg_stat_statements_reset_time.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Feature improvement for pg_stat_statements

Yuki Seino
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
Reply | Threaded
Open this post in threaded view
|

Re: Feature improvement for pg_stat_statements

Kyotaro Horiguchi-4
At Fri, 16 Oct 2020 10:47:50 +0000, Yuki Seino <[hidden email]> 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