Обсуждение: [Patch] Add a reset_computed_values function in pg_stat_statements
Hello pg_stat_statements is a great tool to track performance issue in live databases, especially when adding interfaces like PoWA on top of it. But so far, tools like PoWA can not track the min_time, max_time, mean_time and sum_var_time of queries : these statistics are cumulated over time, fetching points in time would be of little to no use, especially when looking at the impact of a DDL change. This patch thus introduces a simple pg_stat_statements_reset_computed_values function that reset the computed statistics, leaving all other information alive, thus allowing the aforementioned scenario. Regards Pierre Ducroquet
Вложения
Em dom, 1 de set de 2019 às 06:04, Pierre Ducroquet <p.psql@pinaraf.info> escreveu: > > Hello > > pg_stat_statements is a great tool to track performance issue in live > databases, especially when adding interfaces like PoWA on top of it. > But so far, tools like PoWA can not track the min_time, max_time, mean_time > and sum_var_time of queries : these statistics are cumulated over time, > fetching points in time would be of little to no use, especially when looking > at the impact of a DDL change. > This patch thus introduces a simple pg_stat_statements_reset_computed_values > function that reset the computed statistics, leaving all other information > alive, thus allowing the aforementioned scenario. > Pierre, I don't understand why you want to reset part of the statement statistics. If you want the minimal query time every week, reset all statistics of that statement (v12 or later). Postgres provides histogram metrics (min, max, mean, stddev). AFAICS you want a metric type called timer (combination of histogram and meter). For example, calls, sum, min, max, mean, standard deviation will be calculated each hour. If I were to add a new metric to pg_stat_statements, it would be last_time. Compare histogram metrics with the last statement time is useful to check if a certain optimization was effective. Talking about your patch, math is wrong. If you don't reset total_time, all computed values will be incorrect. You are changing the actual meaning of those metrics and I think it is unacceptable because it will break applications. Instead, you should provide (i) new counters or (ii) add GUC to control this behavior. It is a trade-off between incompatibility and too many metrics. Though I wouldn't like to break compatibility (as I said you can always reset all statement statistics). Why don't you provide a pg_stat_statements_reset_computed_values(userid Oid, dbid Oid, queryid bigint)? You forgot to provide documentation for the new function. You should revoke pg_stat_statements_reset_computed_values from PUBLIC. Regards, -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
On Monday, September 2, 2019 3:25:00 AM CEST Euler Taveira wrote: > Em dom, 1 de set de 2019 às 06:04, Pierre Ducroquet > > <p.psql@pinaraf.info> escreveu: > > Hello > > > > pg_stat_statements is a great tool to track performance issue in live > > databases, especially when adding interfaces like PoWA on top of it. > > But so far, tools like PoWA can not track the min_time, max_time, > > mean_time > > and sum_var_time of queries : these statistics are cumulated over time, > > fetching points in time would be of little to no use, especially when > > looking at the impact of a DDL change. > > This patch thus introduces a simple > > pg_stat_statements_reset_computed_values function that reset the computed > > statistics, leaving all other information alive, thus allowing the > > aforementioned scenario. > Hello Thank you for reviewing this patch. > Pierre, I don't understand why you want to reset part of the statement > statistics. If you want the minimal query time every week, reset all > statistics of that statement (v12 or later). Postgres provides > histogram metrics (min, max, mean, stddev). AFAICS you want a metric > type called timer (combination of histogram and meter). For example, > calls, sum, min, max, mean, standard deviation will be calculated each > hour. If I were to add a new metric to pg_stat_statements, it would be > last_time. Compare histogram metrics with the last statement time is > useful to check if a certain optimization was effective. min/max/mean timing in pg_stat_statements are *computed* fields, not simple cumulative ones, hence the distinction I do here. For a tool like PoWA, that (to simplify) copies all the statistics every few minutes to do its own computation, it makes it impossible to get any sense out of them. The facility to reset all statistics is an option, but it feels like an heavy solution when only some statistics are different from the others. > Talking about your patch, math is wrong. If you don't reset > total_time, all computed values will be incorrect. You are changing > the actual meaning of those metrics and I think it is unacceptable > because it will break applications. I think you misunderstood the way this patch is written. total_time, in this function, is not a global statistics: pgss_store receive the total time of the current query. When the computed statistics are reset, we simply drop them and start them fresh from the first query. There is absolutely no compatibility impact here. > Instead, you should provide (i) new counters or (ii) add GUC to control this > behavior. It is a trade-off between incompatibility and too many metrics. > Though I wouldn't like to break compatibility (as I said you can always > reset all statement statistics). As explained: it does not break compatibility, I don't see a need for a new GUC. > Why don't you provide a pg_stat_statements_reset_computed_values(userid Oid, > dbid Oid, queryid bigint)? Because I did not need one, and with pg_stat_statements_reset(userid Oid, dbid Oid, queryid bigint) being newly added, I did not think about it. I can add one in a new version of the patch of course. > You forgot to provide documentation for the new function. Indeed, I am not very used to SGML and first wanted a discussion about the feature. > You should revoke pg_stat_statements_reset_computed_values from PUBLIC. Indeed, I missed that one. Attached to this mail is a new version fixing the revoke issue, and adding documentation. Regards Pierre
Вложения
This patch seems to be failing the contrib build. Please fix. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Sep 26, 2019 at 8:05 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > This patch seems to be failing the contrib build. Please fix. Hi Pierre, In contrib/pg_stat_statements/pg_stat_statements.c, you need to declare or define entry_reset_computed() before you use it. I suppose your compiler must be warning about that. I personally put "COPT=-Wall -Werror" into src/Makefile.custom to make sure that I don't miss any warnings. That's also what http://cfbot.cputube.org/ does (a thing that does a full check-world on all registered patches to look for such problems).
On Tue, Nov 05, 2019 at 11:03:58AM +1300, Thomas Munro wrote: > In contrib/pg_stat_statements/pg_stat_statements.c, you need to > declare or define entry_reset_computed() before you use it. I suppose > your compiler must be warning about that. I personally put > "COPT=-Wall -Werror" into src/Makefile.custom to make sure that I > don't miss any warnings. That's also what http://cfbot.cputube.org/ > does (a thing that does a full check-world on all registered patches > to look for such problems). Still the same problem which has not been addressed after a couple of weeks, so I am marking the entry as returned with feedback. -- Michael