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

On 20 Jun 2018, at 09:30, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

The pg_stat_statements contains the statistics of the queries that are cumulative.
I find that any optimizations that are done to improve the performance of a query
are not be visible clearly until the stats are reset. Currently there is a function to
reset all the statistics, I find it will be useful if we a function that resets the stats of
a single query, instead of reseting all the queries.

I’ve found myself wanting this in the past, so +1 on the idea.

Attached is a simple patch with implementation. Comments?

From only skimming it, the patch seems to lack documentation updates to
doc/src/sgml/pgstatstatements.sgml.

cheers ./daniel

On Jun 20, 2018, at 12:30 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

Attached is a simple patch with implementation. Comments?

Seems useful to me too! What are the odds we could have a column telling the timestamp when a particular query was last reset? Would that be complicated to add?

-Jeremy

Hello

key.userid = GetUserId();

We can remove query id only from current user, right? Maybe better provide optional argument for userid? Typically user with pg_read_all_stats and user for application queries are different users.
At least it should be documented.

regards, Sergei

2018-06-20 4:30 GMT-03:00 Haribabu Kommi <kommi.haribabu@gmail.com>:

Attached is a simple patch with implementation. Comments?

Why don't you extend the existing function pg_stat_statements_reset()?

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

On Wed, Jun 20, 2018 at 1:00 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:

The pg_stat_statements contains the statistics of the queries that are
cumulative.
I find that any optimizations that are done to improve the performance of a
query
are not be visible clearly until the stats are reset. Currently there is a
function to
reset all the statistics, I find it will be useful if we a function that
resets the stats of
a single query, instead of reseting all the queries.

Attached is a simple patch with implementation. Comments?

The idea looks interesting to me. But, as Euler Taveira mentioned,
can't we extend an existing function pg_stat_statements_reset()
instead of adding a new one and update the documentation for it. Also,
in the test-case it would be good to display the output of
pg_stat_statements before and after deleting the query. Thanks.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Wed, Jun 20, 2018 at 10:19 AM, Euler Taveira <euler@timbira.com.br> wrote:

2018-06-20 4:30 GMT-03:00 Haribabu Kommi <kommi.haribabu@gmail.com>:

Attached is a simple patch with implementation. Comments?

Why don't you extend the existing function pg_stat_statements_reset()?

Well, the existing function doesn't take any arguments. We could add
an additional version of it that takes an argument, or we could
replace the existing version with one that has an optional argument.
But are either of those things any better than just adding a new
function with a different name, like
pg_stat_statements_reset_statement()?

I have not had such good experiences with function overloading, either
in PostgreSQL or elsewhere, that I'm ready to say reusing the same
name is definitely the right approach. For example, suppose we
eventually end up with a function that resets all the statements, a
function that resets just one statement, a function that resets all
statements for one user, and a function that resets all statements
where the statement text matches a certain regexp. If those all have
separate names, everything is fine. If they all have the same name,
there's no way that's not confusing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Why not just parameterize it with the three key fields; userid, dbid, and queryid?

i.e It would then allow it be limited to only records associated with a given user and/or database as well.

pg_stat_statements_reset(dbid oid, userid oid, queryid bigint)

pg_stat_statements_reset(null, null, 3569076157) — all for a given query
pg_stat_statements_reset(6384, null, 3569076157)
pg_stat_statements_reset(null, 16429, 3569076157)
pg_stat_statements_reset(6384, 6384, 3569076157)
pg_stat_statements_reset(6384, null, null) — all for a given database.
.
.
.

2018-06-22 12:06 GMT-03:00 Robert Haas <robertmhaas@gmail.com>:

On Wed, Jun 20, 2018 at 10:19 AM, Euler Taveira <euler@timbira.com.br> wrote:

2018-06-20 4:30 GMT-03:00 Haribabu Kommi <kommi.haribabu@gmail.com>:

Attached is a simple patch with implementation. Comments?

Why don't you extend the existing function pg_stat_statements_reset()?

Well, the existing function doesn't take any arguments. We could add
an additional version of it that takes an argument, or we could
replace the existing version with one that has an optional argument.
But are either of those things any better than just adding a new
function with a different name, like
pg_stat_statements_reset_statement()?

From the user's POV, overloading is a good goal. It is better to
remember one function name than 3 different function names to do the
same task (reset statement statistics).

I have not had such good experiences with function overloading, either
in PostgreSQL or elsewhere, that I'm ready to say reusing the same
name is definitely the right approach. For example, suppose we
eventually end up with a function that resets all the statements, a
function that resets just one statement, a function that resets all
statements for one user, and a function that resets all statements
where the statement text matches a certain regexp. If those all have
separate names, everything is fine. If they all have the same name,
there's no way that's not confusing.

I think part of your frustration with overloading is because there are
so many arguments and/or argument type combinations to remember.
Frankly, I never remember the order / type of arguments when a
function has more than 2 arguments (unless I use that function a lot).
If we want to consider overloading I think we should put all
possibilities for that function on the table and decide between
overload it or not. Bad overloading decisions tend to be very
frustrating for the user. In this case, all possibilities (queryid,
user, regex, database, statement regex) can be summarized as (i) 0
arguments that means all statements and (ii) 1 argument (queryid is
unique for all statements) -- because queryid can be obtain using
SELECT pg_stat_statements_reset(queryid) FROM pg_stat_statements WHERE
my-condition-here.

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

On Sat, Jun 23, 2018 at 5:45 AM Euler Taveira <euler@timbira.com.br> wrote:

2018-06-22 12:06 GMT-03:00 Robert Haas <robertmhaas@gmail.com>:

On Wed, Jun 20, 2018 at 10:19 AM, Euler Taveira <euler@timbira.com.br>

wrote:

2018-06-20 4:30 GMT-03:00 Haribabu Kommi <kommi.haribabu@gmail.com>:

Attached is a simple patch with implementation. Comments?

Why don't you extend the existing function pg_stat_statements_reset()?

Well, the existing function doesn't take any arguments. We could add
an additional version of it that takes an argument, or we could
replace the existing version with one that has an optional argument.
But are either of those things any better than just adding a new
function with a different name, like
pg_stat_statements_reset_statement()?

From the user's POV, overloading is a good goal. It is better to
remember one function name than 3 different function names to do the
same task (reset statement statistics).

I agree that function overloading is beneficial until unless it doesn't
introduce
confusion and difficulty in using it.

I have not had such good experiences with function overloading, either

in PostgreSQL or elsewhere, that I'm ready to say reusing the same
name is definitely the right approach. For example, suppose we
eventually end up with a function that resets all the statements, a
function that resets just one statement, a function that resets all
statements for one user, and a function that resets all statements
where the statement text matches a certain regexp. If those all have
separate names, everything is fine. If they all have the same name,
there's no way that's not confusing.

I think part of your frustration with overloading is because there are
so many arguments and/or argument type combinations to remember.
Frankly, I never remember the order / type of arguments when a
function has more than 2 arguments (unless I use that function a lot).
If we want to consider overloading I think we should put all
possibilities for that function on the table and decide between
overload it or not. Bad overloading decisions tend to be very
frustrating for the user. In this case, all possibilities (queryid,
user, regex, database, statement regex) can be summarized as (i) 0
arguments that means all statements and (ii) 1 argument (queryid is
unique for all statements) -- because queryid can be obtain using
SELECT pg_stat_statements_reset(queryid) FROM pg_stat_statements WHERE
my-condition-here.

In pg_stat_statements the uniqueness of the query is decided by (userid,
dbid and queryid).
I feel the reset function should take maximum of 3 input arguments and rest
of the
conditions can be filtered using the WHERE condition.

if we are going to support a single function that wants to reset all the
statement
statistics of a "user" or specific to a "database" and "specific queries"
based on
other input parameter values, I am not sure that the overloading function
can cause
confusion in using it? And also if the specified input query data doesn't
found, better
to ignore or raise an error?

Regards,
Haribabu Kommi
Fujitsu Australia

On Sat, Jun 23, 2018 at 7:36 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:

On Sat, Jun 23, 2018 at 5:45 AM Euler Taveira <euler@timbira.com.br> wrote:

2018-06-22 12:06 GMT-03:00 Robert Haas <robertmhaas@gmail.com>:

On Wed, Jun 20, 2018 at 10:19 AM, Euler Taveira <euler@timbira.com.br>
wrote:

2018-06-20 4:30 GMT-03:00 Haribabu Kommi <kommi.haribabu@gmail.com>:

Attached is a simple patch with implementation. Comments?

Why don't you extend the existing function pg_stat_statements_reset()?

Well, the existing function doesn't take any arguments. We could add
an additional version of it that takes an argument, or we could
replace the existing version with one that has an optional argument.
But are either of those things any better than just adding a new
function with a different name, like
pg_stat_statements_reset_statement()?

From the user's POV, overloading is a good goal. It is better to
remember one function name than 3 different function names to do the
same task (reset statement statistics).

I agree that function overloading is beneficial until unless it doesn't
introduce
confusion and difficulty in using it.

I have not had such good experiences with function overloading, either
in PostgreSQL or elsewhere, that I'm ready to say reusing the same
name is definitely the right approach. For example, suppose we
eventually end up with a function that resets all the statements, a
function that resets just one statement, a function that resets all
statements for one user, and a function that resets all statements
where the statement text matches a certain regexp. If those all have
separate names, everything is fine. If they all have the same name,
there's no way that's not confusing.

I think part of your frustration with overloading is because there are
so many arguments and/or argument type combinations to remember.
Frankly, I never remember the order / type of arguments when a
function has more than 2 arguments (unless I use that function a lot).
If we want to consider overloading I think we should put all
possibilities for that function on the table and decide between
overload it or not. Bad overloading decisions tend to be very
frustrating for the user. In this case, all possibilities (queryid,
user, regex, database, statement regex) can be summarized as (i) 0
arguments that means all statements and (ii) 1 argument (queryid is
unique for all statements) -- because queryid can be obtain using
SELECT pg_stat_statements_reset(queryid) FROM pg_stat_statements WHERE
my-condition-here.

In pg_stat_statements the uniqueness of the query is decided by (userid,
dbid and queryid).
I feel the reset function should take maximum of 3 input arguments and rest
of the
conditions can be filtered using the WHERE condition.

I'm slightly confused about the function overloading part here (not
sure whether we should try using the same function name or introduce a
function with new name), however, I think, passing userid, dbid and
queryid as input arguments to the user function (may be
pg_stat_statements_reset_statement or pg_stat_statements_reset) looks
like a good option as it would allow users to remove an entry for a
particular query across the databases not just in the database that
the user is currently connected to. The current patch definitely lacks
this flexibility.

On Sat, Jun 23, 2018 at 2:52 PM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

On Sat, Jun 23, 2018 at 7:36 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:

On Sat, Jun 23, 2018 at 5:45 AM Euler Taveira <euler@timbira.com.br>

wrote:

2018-06-22 12:06 GMT-03:00 Robert Haas <robertmhaas@gmail.com>:

On Wed, Jun 20, 2018 at 10:19 AM, Euler Taveira <euler@timbira.com.br

wrote:

Why don't you extend the existing function

pg_stat_statements_reset()?

Well, the existing function doesn't take any arguments. We could add
an additional version of it that takes an argument, or we could
replace the existing version with one that has an optional argument.
But are either of those things any better than just adding a new
function with a different name, like
pg_stat_statements_reset_statement()?

From the user's POV, overloading is a good goal. It is better to
remember one function name than 3 different function names to do the
same task (reset statement statistics).

I agree that function overloading is beneficial until unless it doesn't
introduce
confusion and difficulty in using it.

I have not had such good experiences with function overloading, either
in PostgreSQL or elsewhere, that I'm ready to say reusing the same
name is definitely the right approach. For example, suppose we
eventually end up with a function that resets all the statements, a
function that resets just one statement, a function that resets all
statements for one user, and a function that resets all statements
where the statement text matches a certain regexp. If those all have
separate names, everything is fine. If they all have the same name,
there's no way that's not confusing.

I'm slightly confused about the function overloading part here (not
sure whether we should try using the same function name or introduce a
function with new name), however, I think, passing userid, dbid and
queryid as input arguments to the user function (may be
pg_stat_statements_reset_statement or pg_stat_statements_reset) looks
like a good option as it would allow users to remove an entry for a
particular query across the databases not just in the database that
the user is currently connected to. The current patch definitely lacks
this flexibility.

Thanks for all your comments on the and approach. Patch is changed
to use the existing _reset function and it takes 3 arguments userid, dbid
and queryid
and their default values are 0.

If all values are passed, it resets only a single query statement,
otherwise it resets
all the statements that matches with the other parameters. If no values are
passed
or all values are invalid, all the statistics are reset.

Updated patch attached.

Regards,
Haribabu Kommi
Fujitsu Australia

Hello
Thank you for update

It may be better to use NULL as the default value at sql level.

ereport(LOG, (errmsg("userid %u, dbid %u, queryid %ld does not exist", userid, dbid, queryid)));

I think LOG level is not useful here. In common case this is server log only. How about WARNING? Or just ignore. Want remove row? Here is no such row anymore, all fine.
Also we can return num_remove instead of void. I think this is even better. But this break backward compatibility and we need something like pg_stat_statements_reset_1_6

By default, this function can only be executed by superusers.

Can you also update this phrase in pg_stat_statements_reset documentation? Beginning from 1.5 version this is not true, reset can be used by any user with pg_read_all_stats role.

regards, Sergei

On 29/06/18 10:14, Sergei Kornilov wrote:

It may be better to use NULL as the default value at sql level.

Absolutely. +1 from me.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Fri, Jun 29, 2018 at 6:14 PM Sergei Kornilov <sk@zsrv.org> wrote:

Hello
Thank you for update

Thanks for the review.

It may be better to use NULL as the default value at sql level.

Changed.

ereport(LOG, (errmsg("userid %u, dbid %u, queryid %ld does not exist",

userid, dbid, queryid)));
I think LOG level is not useful here. In common case this is server log
only. How about WARNING? Or just ignore. Want remove row? Here is no such
row anymore, all fine.

I went with removing of ereport.

Also we can return num_remove instead of void. I think this is even
better. But this break backward compatibility and we need something like
pg_stat_statements_reset_1_6

Changed. Returning removed rows is nice than just void.

By default, this function can only be executed by superusers.

Can you also update this phrase in pg_stat_statements_reset documentation?
Beginning from 1.5 version this is not true, reset can be used by any user
with pg_read_all_stats role.

Doc is updated and also separate thread is started for the doc fix [1]/messages/by-id/CAJrrPGcim=Q-7ewhuKr1n3mkeELySC-QeVHGZJJYwaaKMSJRkg@mail.gmail.com.
Updated patch attached.

[1]: /messages/by-id/CAJrrPGcim=Q-7ewhuKr1n3mkeELySC-QeVHGZJJYwaaKMSJRkg@mail.gmail.com
/messages/by-id/CAJrrPGcim=Q-7ewhuKr1n3mkeELySC-QeVHGZJJYwaaKMSJRkg@mail.gmail.com

Regards,
Haribabu Kommi
Fujitsu Australia

Hello

I found SELECT pg_stat_statements_reset(NULL,NULL,s.queryid) in tests and it pass tests, but i wonder how it works. Should not we check the NULL through PG_ARGISNULL macro before any PG_GETARG_*? According src/include/fmgr.h

* If function is not marked "proisstrict" in pg_proc, it must check for
* null arguments using this macro. Do not try to GETARG a null argument!

pg_stat_statements_reset(userid Oid, dbid Oid, queryid bigint) returns void

And you forgot to change return type in docs (and description of return value)

regards, Sergei

On Mon, Jul 2, 2018 at 6:42 PM Sergei Kornilov <sk@zsrv.org> wrote:

Hello

Thanks for the review.

I found SELECT pg_stat_statements_reset(NULL,NULL,s.queryid) in tests and
it pass tests, but i wonder how it works. Should not we check the NULL
through PG_ARGISNULL macro before any PG_GETARG_*? According
src/include/fmgr.h

* If function is not marked "proisstrict" in pg_proc, it must check for
* null arguments using this macro. Do not try to GETARG a null argument!

Thanks for checking, Added.

pg_stat_statements_reset(userid Oid, dbid Oid, queryid bigint) returns

void
And you forgot to change return type in docs (and description of return
value)

Corrected and also added details of the returns value.

Update patch attached.

Regards,
Haribabu Kommi
Fujitsu Australia

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

I reviewed and tested patch one more times, including check with previous extension version. I have no more notes on the code. Patch has tests and appropriate documentation changes.
I think the patch is ready for committer.

regards, Sergei

The new status of this patch is: Ready for Committer

On Wed, Jul 4, 2018 at 7:12 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

On Mon, Jul 2, 2018 at 6:42 PM Sergei Kornilov <sk@zsrv.org> wrote:

Hello

Thanks for the review.

I found SELECT pg_stat_statements_reset(NULL,NULL,s.queryid) in tests and
it pass tests, but i wonder how it works. Should not we check the NULL
through PG_ARGISNULL macro before any PG_GETARG_*? According
src/include/fmgr.h

* If function is not marked "proisstrict" in pg_proc, it must check for
* null arguments using this macro. Do not try to GETARG a null
argument!

Thanks for checking, Added.

pg_stat_statements_reset(userid Oid, dbid Oid, queryid bigint) returns
void

And you forgot to change return type in docs (and description of return
value)

Corrected and also added details of the returns value.

Update patch attached.

+ if (userid != 0 && dbid != 0 && queryid != 0)

UINT64CONST() should be used for the constant for queryid?

It's rare case, but 0 can be assigned as valid queryid. Right?
If yes, it's problematic to use 0 as an invalid queryid here.

+      By default, this function can only be executed by superusers and members
+      of the <literal>pg_read_all_stats</literal> role.

Currently pg_stat_statements_reset() and other stats reset functions like
pg_stat_reset() can be executed only by superusers.
But why did you allow pg_read_all_stats role to execute that function,
by default? That change looks strange to me.

Regards,

--
Fujii Masao

Hello

Currently pg_stat_statements_reset() and other stats reset functions like
pg_stat_reset() can be executed only by superusers.
But why did you allow pg_read_all_stats role to execute that function,
by default? That change looks strange to me.

This is not behavior change, only fix documentation to current state. Commit https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=25fff40798fc4ac11a241bfd9ab0c45c085e2212 creates 1.5 version and grant execute to pg_read_all_stats

Backpatch proposal was started in separate thread /messages/by-id/CAJrrPGcim=Q-7ewhuKr1n3mkeELySC-QeVHGZJJYwaaKMSJRkg@mail.gmail.com

regards, Sergei