On Fri, Apr 01, 2022 at 10:47:02PM +0300, Andrei Zubkov wrote:
>
> On Fri, 2022-04-01 at 11:38 -0400, Greg Stark wrote:
> > [13:19:51.544] pg_stat_statements.c: In function ‘entry_reset’:
> > [13:19:51.544] pg_stat_statements.c:2598:32: error:
> > ‘minmax_stats_reset’ may be used uninitialized in this function
> > [-Werror=maybe-uninitialized]
> > [13:19:51.544] 2598 | entry->minmax_stats_since = minmax_stats_reset;
> > [13:19:51.544] | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> >
>
> I was afraid of such warning can appear..
>
> On Sat, 2022-04-02 at 00:13 +0800, Julien Rouhaud wrote:
> > I guess that pg_stat_statement_reset() is so
> > expensive that an extra gettimeofday() wouldn't change much.
>
> Agreed
>
> > Otherwise
> > initializing to NULL should be enough.
>
> Julien, I would prefer an extra GetCurrentTimestamp(). So, I've opted
> to use the common unconditional
>
> stats_reset = GetCurrentTimestamp();
>
> for an entire entry_reset function due to the following:
>
> 1. It will be uniform for stats_reset and minmax_stats_reset
> 2. As you mentioned, it wouldn't change a much
> 3. The most common way to use this function is to reset all statements
> meaning that GetCurrentTimestamp() will be called anyway to update the
> value of stats_reset field in pg_stat_statements_info view
> 4. Actually I would like that pg_stat_statements_reset function was
> able to return the value of stats_reset as its result. This could give
> to the sampling solutions the ability to check if the last reset (of
> any type) was performed by this solution or any other reset was
> performed by someone else. It seems valuable to me, but it changes the
> result type of the pg_stat_statements_reset() function, so I don't know
> if we can do that right now.
I'm fine with always getting the current timestamp when calling the function.
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.
