Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements - Mailing list pgsql-hackers

Hi Julien!

Thank you for such detailed review!

On Wed, 2022-03-30 at 17:31 +0800, Julien Rouhaud wrote:
> 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

Yes, I've just added some tests there, but it seems they are not quite
suficient. Maybe we should try to do some queries to views and
functions in old versions? A least when new C function version
appears...

During tests developing I've noted that current test of
pg_stat_statements_info view actually tests only view access. However
we can test at least functionality of stats_reset field like this:

SELECT now() AS ref_ts \gset
SELECT dealloc, stats_reset >= :'ref_ts' FROM pg_stat_statements_info;
SELECT pg_stat_statements_reset();
SELECT dealloc, stats_reset >= :'ref_ts' FROM pg_stat_statements_info;

Does it seems reasonable? 

> - the last test removed the minmax_plan_zero field, why?

My thaught was as follows... Reexecution of the same query will
definitely cause execution. However, most likely it wouldn't be
planned, but if it would (maybe this is possible, or maybe it will be
possible in the future in some cases) the test shouldn't fail. Checking
of only execution stats seems enough to me - in most cases we can't
check planning stats with such test anyway.
What do you think about it?

> 
> Code:
> 
> +       TimestampTz     stats_since;            /* timestamp of entry
> allocation moment */
> 
> I think "timestamp of entry allocation" is enough?

Yes

> 
> +                        * 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"

Agreed

> 
> +/*
> + * 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? 

Great point! I've merged minmax reset functionality in the entry_reset
function.

> 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
> )

Of course!

> 
> 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.

Agreed.

I've attached 7th patch version with fixes mentioned above.
-- 
Best regards, Andrei Zubkov