Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements

On Thu, Mar 31, 2022 at 01:06:10PM +0300, Andrei Zubkov wrote:
>
> 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...

I'm not sure if that's really helpful. If you have new C functions and old
SQL-version, you won't be able to reach them anyway. Similarly, if you have
the new SQL but the old .so (which we can't test), it will just fail as the
symbol doesn't exist. The real problem that has to be explicitly handled by
the C code is different signatures for C functions.
>
> 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?

It looks reasonable, especially if the patch adds a new mode for the reset
function.

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

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.

Thanks for the work on merging the functions! I will reply on the other parts
of the thread where some discussion started.