Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements

Hi,

On Fri, Mar 25, 2022 at 01:25:23PM +0300, Andrei Zubkov wrote:
> Greg, thank you for your attention and for your thought.
>
> I've just completed the 6th version of a patch implementing idea
> proposed by Julien Rouhaud, i.e. without auxiliary statistics. 6th
> version will reset current min/max fields to zeros until the first plan
> or execute.

Thanks!

I've decided to use zeros here because planning statistics
> is zero in case of disabled tracking. I think sampling solution could
> easily handle this.

I'm fine with it. It's also consistent with the planning counters when
track_planning is disabled. And even if the sampling solution doesn't handle
it, you will simply get consistent values, like "0 calls with minmax timing of
0 msec", so it's not really a problem.

Feature wise, I'm happy with the patch. I just have a few comments.

Tests:

- it's missing some test in sql/oldextversions.sql to validate that the code
works with the extension in version 1.9
- the last test removed the minmax_plan_zero field, why?

Code:

+ TimestampTz stats_since; /* timestamp of entry allocation moment */

I think "timestamp of entry allocation" is enough?

+ * Calculate min and max time. min = 0 and max = 0
+ * means that min/max statistics reset was happen

maybe "means that the min/max statistics were reset"

+/*
+ * Reset min/max values of specified entries
+ */
+static void
+entry_minmax_reset(Oid userid, Oid dbid, uint64 queryid)
+{
[...]

There's a lot of duplicated logic with entry_reset().
Would it be possible to merge at least the C reset function to handle either
all-metrics or minmax-only? Also, maybe it would be better to have a single SQL
reset function, something like:

pg_stat_statements_reset(IN userid Oid DEFAULT 0,
IN dbid Oid DEFAULT 0,
IN queryid bigint DEFAULT 0,
IN minmax_only DEFAULT false
)

Doc:

+ <structfield>stats_since</structfield> <type>timestamp with time zone</type>
+ </para>
+ <para>
+ Timestamp of statistics gathering start for the statement

The description is a bit weird. Maybe like "Time at which statistics gathering
started for this statement"? Same for the minmax version.