Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements

Hi,

On Fri, 2022-04-01 at 13:01 -0700, Andres Freund wrote:
> It seems decidedly not great to have four copies of this code. It was
> already
> not great before, but this patch makes the duplicated section go from
> four
> lines to 20 or so.

Agreed. I've created the single_entry_reset() function wrapping this
code. I wonder if it should be declared as inline to speedup a little.

On Sat, 2022-04-02 at 15:10 +0800, Julien Rouhaud wrote:
> > 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?
> 
> It looks reasonable, especially if the patch adds a new mode for the
> reset
> function.

I've implemented this test.

> > 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?
> 
> Ah I see.  I guess we could set plan_cache_mode to force_generic_plan
> to make
> sure we go though planning.  But otherwise just adding a comment
> saying that
> the test has to be compatible with different plan caching approach
> would be
> fine with me.

Set plan_cache_mode seems a little bit excess to me. And maybe in the
future some another plan caching strategies will be implementd with
coresponding settings.. So I've just left a comment there.

On Sat, 2022-04-02 at 15:21 +0800, Julien Rouhaud wrote:
> I'm not sure about returning the ts.  If you need it you could call
> SELECT
> now() FROM pg_stat_statements_reset() (or clock_timestamp()).  It
> won't be
> entirely accurate but since the function will have an exclusive lock
> during the
> whole execution that shouldn't be a problem.  Now you're already
> adding a new
> version of the C function so I guess that it wouldn't require any
> additional
> effort so why not.

I think that if we can do it in accurate way and there is no obvious
side effects, why not to try it...
Changing of pg_stat_statements_reset function result caused a
confiderable tests update. Also, I'm not sure that my description of
this feature in the docs is blameless..

After all, I'm a little bit in doubt about this feature, so I'm ready
to rollback it.

v9 attached
--
regards, Andrei