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.