Re: [PATCH] pg_stat_statements dealloc field ignores manual deallocation
On Fri, 2021-03-19 at 22:15 +0800, Julien Rouhaud wrote: > I disagree. The point of that field is to help users configuring > pg_stat_statements.max, as evictions have a huge overhead in many > workloads. > > If users remove entries for some reasons, we don't have to give the > impression > that pg_stat_statements.max is too low and that it should be > increased, > especially since it requires a restart. > Ok. But when we are collecting aggregated statistics on pg_stat_statememts periodically it would be great to know about every deallocation happened. Maybe we need to add another counter for manual deallocations tracking?
Re: [PATCH] pg_stat_statements dealloc field ignores manual deallocation
On Fri, Mar 19, 2021 at 05:08:45PM +0300, Андрей Зубков wrote: > > Since 2e0fedf there is a view pg_stat_statements_info is available in > pg_stat_statements extension. It has a dealloc field, that should be a > counter of deallocation events happened. > Right now it accounts only automatic deallocation events, happened when > we need a place for a new statement, Yes, and that behavior is documented: dealloc bigint Total number of times pg_stat_statements entries about the least-executed statements were deallocated because more distinct statements than pg_stat_statements.max were observed > but manual deallocation events > caused by pg_stat_statements_reset() function for some subset of > collected statements is not accounted. > My opinion is that manual deallocation is a deallocation too and it > must be accounted in dealloc field of pg_stat_statements_info view. I disagree. The point of that field is to help users configuring pg_stat_statements.max, as evictions have a huge overhead in many workloads. If users remove entries for some reasons, we don't have to give the impression that pg_stat_statements.max is too low and that it should be increased, especially since it requires a restart.
[PATCH] pg_stat_statements dealloc field ignores manual deallocation
Hi, This is a proposal of a patch for pg_stat_statements extension. It corrects deallocation events accounting. Since 2e0fedf there is a view pg_stat_statements_info is available in pg_stat_statements extension. It has a dealloc field, that should be a counter of deallocation events happened. Right now it accounts only automatic deallocation events, happened when we need a place for a new statement, but manual deallocation events caused by pg_stat_statements_reset() function for some subset of collected statements is not accounted. My opinion is that manual deallocation is a deallocation too and it must be accounted in dealloc field of pg_stat_statements_info view. Let's see how it happens: postgres=# select pg_stat_statements_reset(); postgres=# select 1; ?column? -- 1 (1 row) postgres=# select dealloc from pg_stat_statements_info ; dealloc - 0 (1 row) postgres=# select pg_stat_statements_reset(userid,dbid,queryid) postgres-# from pg_stat_statements where query = 'select $1'; pg_stat_statements_reset -- (1 row) postgres=# select dealloc from pg_stat_statements_info ; dealloc - 0 -- Here must be a one now, as deallocation happened (1 row) This patch adds accounting of manual deallocation events. -- Andrei Zubkov Postgres Professional The Russian Postgres Company diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 62cccbfa44..184e5c08ea 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2579,7 +2579,18 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid) /* All entries are removed? */ if (num_entries != num_remove) + { + if (num_remove) + { + /* Increment the number of times entries are deallocated */ + volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; + + SpinLockAcquire(>mutex); + s->stats.dealloc += 1; + SpinLockRelease(>mutex); + } goto release_lock; + } /* * Reset global statistics for pg_stat_statements since all entries are