Re: Feature improvement for pg_stat_statements

At Fri, 16 Oct 2020 10:47:50 +0000, Yuki Seino  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, pg_stat_statements,
>     -   and the utility functions pg_stat_statements_reset and
>     -   pg_stat_statements.  These are not available globally but
>     -   can be enabled for a specific database with
>     +   these statistics, the module provides views, pg_stat_statements
>     +   and pg_stat_statements_ctl,
>     +   and the utility functions pg_stat_statements_reset,
>     +   pg_stat_statements, and pg_stat_statements_reset_time.
>     +   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 pg_stat_statements_reset(0,0,0) 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