At Fri, 16 Oct 2020 10:47:50 +0000, Yuki Seino <sein...@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
