On Wed, Jan 9, 2019 at 2:25 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> Few minor comments: > Thanks for the review. > 1. > bench=# SELECT query, calls, total_time, rows, 100.0 * shared_blks_hit / > nullif(shared_blks_hit + shared_blks_read, 0) AS hit_percent > FROM pg_stat_statements ORDER BY total_time DESC LIMIT 5; > > -[ RECORD 5 > ]-------------------------------------------------------------------- > query | vacuum analyze pgbench_accounts > calls | 1 > total_time | 136.448116 > rows | 0 > hit_percent | 99.9201915403032721 > > There is nothing between the first and second time you ran this query, > so where did this vacuum analyze .. record came? > Because of the pg_stat_statements_reset() function call to reset the first query, when the query executed second time the above statement is appeared as the select query is limiting the result to 5. > 2. > bench=# SELECT pg_stat_statements_reset(0,0,s.queryid) FROM > pg_stat_statements AS s > bench-# WHERE s.query = 'UPDATE pgbench_branches SET bbalance = > bbalance + $1 WHERE bid = $2'; > > bench=# > > I think it would be good if you show the output of > pg_stat_statements_reset statement instead of showing empty line. > The pg_stat_statements_reset() function just returns void, because of this reason, already existing _reset() call also just lists the empty line, I also followed the same. I feel it is better to add the return data for all the _reset() function calls or leave it as empty. > Another minor point is that in the second statement > (pg_stat_statements_reset), you seem to have made it a two-line > statement whereas first one looks to be a single-line statement, it > would be good from the readability point of view if both looks same. > I would prefer the second to look similar to the first one. > OK. Corrected. Updated patch attached. Regards, Haribabu Kommi Fujitsu Australia
0001-Extend-pg_stat_statements_reset-to-reset-statistics_v16.patch
Description: Binary data
