Re: Feature improvement for pg_stat_statements

At Fri, 16 Oct 2020 10:47:50 +0000, Yuki Seino <seinoyu(at)oss(dot)nttdata(dot)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