Re: Feature improvement for pg_stat_statements

At Fri, 16 Oct 2020 10:47:50 +0000, Yuki Seino <seinoyu@oss.nttdata.com> wrote in 
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, failed
> Spec compliant:           tested, passed
> Documentation:            tested, failed
> 
> The patch applies cleanly and looks fine to me.I'm going to list a few points that I think need to be fixed.
> 
> 1.There are unnecessary difference lines 89 in "pg_stat_statements.c".
> 2.It is confusing that the initial value of reset_time is the current date and time, so why not set it to null?
> 3.How about pg_stat_statements_reset_time creates a view?
> 4.How about counting the number of resets?
> 5."pgstatstatstatements.sgml" would need to include the function name in the following section.
>     -   these statistics, the module provides a view, <structname>pg_stat_statements</structname>,
>     -   and the utility functions <function>pg_stat_statements_reset</function> and
>     -   <function>pg_stat_statements</function>.  These are not available globally but
>     -   can be enabled for a specific database with
>     +   these statistics, the module provides views, <structname>pg_stat_statements</structname>
>     +   and <structname>pg_stat_statements_ctl</structname>,
>     +   and the utility functions <function>pg_stat_statements_reset</function>,
>     +   <function>pg_stat_statements</function>, and <function>pg_stat_statements_reset_time</function>.
>     +   These are not available globally but can be enabled for a specific database with
> 
> It would be nice to be able to keep the timing of resetting the userid, dbid and queryid as well, but I think the
statusquo is appropriate for management in memory.
 
> 
> The new status of this patch is: Waiting on Author


+        SpinLockAcquire(&pgss->mutex);

You might noticed, but there a purpose of using the following
idiom. Without that, compiler might optimize out the comparison
assuming *pgss won't change.

>    volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; \
>    SpinLockAcquire(&s->mutex); \

+        SpinLockAcquire(&pgss->mutex);
+        pgss->reset_time = GetCurrentTimestamp();

We should avoid (possiblly) time-cosuming thing like GetCurrentTimestamp();


+      this function shows last reset time only when <function>pg_stat_statements_reset(0,0,0)</function> is called.

This reads as "この関数はpg_statstatement_reset(0,0,0) が呼び出された
ときにだけ最終リセット時刻を表示します。", which I think is different
from what is intentended.

And the wording is not alike with the documentation for similar functions.

https://www.postgresql.org/docs/13/functions-datetime.html
> current_timestamp → timestamp with time zone
> Current date and time (start of current transaction); see Section 9.9.4

https://www.postgresql.org/docs/13/monitoring-stats.html
pg_stat_archiver view
> stats_reset timestamp with time zone
> Time at which these statistics were last reset

So something like the following?

Time at which pg_stat_statements_reset(0,0,0) was last called.

or

Time at which statistics are last discarded by calling pg_stat_statements_reset(0,0,0).


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center