Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements

Hi Julien,

On Sun, 2022-04-03 at 15:07 +0800, Julien Rouhaud wrote:
> +static TimestampTz
> +entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
>  {
>     HASH_SEQ_STATUS hash_seq;
>     pgssEntry  *entry;
> +   Counters   *entry_counters;
>
> Do we really need an extra variable?  Why not simply using
> entry->counters.xxx_time[kind]?
>
> Also, I think it's better to make the macro more like function
> looking, so
> SINGLE_ENTRY_RESET().

Agreed with the both, I'll fix it.

>
> index f2e822acd3..c2af29866b 100644
> --- a/contrib/pg_stat_statements/sql/oldextversions.sql
> +++ b/contrib/pg_stat_statements/sql/oldextversions.sql
> @@ -36,4 +36,12 @@ AlTER EXTENSION pg_stat_statements UPDATE TO
> '1.8';
>  \d pg_stat_statements
>  SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
>
> +ALTER EXTENSION pg_stat_statements UPDATE TO '1.9';
> +\d pg_stat_statements
> +\d pg_stat_statements_info
> +SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
>
> I don't think this bring any useful coverage.

I feel the same, but I've done it like previous tests (versions 1.7 and
1.8). Am I miss something here? Do you think we should remove these
tests completly?

>
> I think this need some rewording (and s/fist/first).  Maybe:
>
> Minimum time spent planning the statement, in milliseconds.
>
> This field will be zero if
> <varname>pg_stat_statements.track_planning</varname>
> is disabled, or if the counter has been reset using the the
> <function>pg_stat_statements_reset</function> function with the
> <structfield>minmax_only</structfield> parameter set to
> <literal>true</literal>
> and never been planned since.

Thanks a lot!

>
>        <primary>pg_stat_statements_reset</primary>
>       </indexterm>
> @@ -589,6 +623,20 @@
>        If all statistics in the
> <filename>pg_stat_statements</filename>
>        view are discarded, it will also reset the statistics in the
>        <structname>pg_stat_statements_info</structname> view.
> +      When <structfield>minmax_only</structfield> is
> <literal>true</literal> only the
> +      values of minimun and maximum execution and planning time will
> be reset (i.e.
>
> Nitpicking: I would say planning and execution time, as the fields
> are in this
> order in the view/function.

Agreed.
--
regards, Andrei