Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

On Thu, Nov 8, 2018 at 4:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sun, Nov 4, 2018 at 6:37 AM Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
> wrote:
> >
> > On Sun, Nov 4, 2018 at 11:17 AM Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
> >>
> >> On Sat, Nov 03, 2018 at 03:56:14PM +0530, Amit Kapila wrote:
> >> > Before trying out any solution or deciding which is better, I think we
> >> > want to understand why the variability in results occurred only after
> >> > your patch? Without the patch, it works just fine.
> >>
> >> Good point. We surely want to have a stable feature, which gets tested
> >> without triggering random failures in the builfarm.
> >
> >
> > Thanks for the review.
> >
> > This patch has changed the pg_stat_statements_reset() function from
> returning void
> > to number statements that it reset.
> >
>
> What is the motivation of that change? It seems to be
> backward-incompatible change. I am not telling we can't do it, but do
> we have strong justification? One reason, I could see is to allow the
> user to get the exact value of statements that got reset and maybe
> that is more helpful as we have now additional parameters in the API,
> but I am not sure if that is a good justification.
>

Yes, as you said that is the main reason to modify the function to return
the number of statements that it reset. Without having any output from
the function, there is no way that how many statements that the above
function reset. Earlier it used to reset every statement, so I feel there
is
no need of any output, but I feel giving the number of statements with
this approach is good.

> > The regression test contains pg_stat_statements_reset()
> > as first query to reset any of the query stats that are already tracked
> to let the test to
> > provide the proper results. But with this feature, if we test this
> regression test on an
> > already running server, the first query result is varying and it leads
> to test failure.
> >
> > So to fix this problem, I added a wrapper function that masks the result
> of the
> > pg_stat_statements_reset() function and just return as void, with this
> wrapper function
> > used a first statement, the test is stable, as this function takes care
> of resetting already
> > existing statements from the already running server.
> >
>
> Or you could change the query to something like (Select NULL from
> pg_stat_statements_reset())
>

Thanks. I changed it accordingly.

> > With the above change, the regression test is stable. Comments?
> >
>
> In the comment message, you wrote: "Backward Compatible change, Now
> pg_stat_statements_reset() function returns the number of statement
> entries that are reset."
>
> Do you want to say incompatible instead of compatible?
>

Yes, your are correct.

> + If no parameter is specified or specify everything as
> <literal>NULL</literal>(invalid),
> + it will discard all statistics.
> + By default, this function can only be executed by superusers.
> Access may
> + be granted to others using <command>GRANT</command>.
>
> I think it will look better if you can continue the "By default, ..."
> line from where the previous line ends rather than starting a new
> line.
>

Updated as per your suggestion.
Attached an updated with above comments correction.

Regards,
Haribabu Kommi
Fujitsu Australia