[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
Attachments:
pg_stat_statements_dealloc_fix_patch_2021_0319.patchtext/x-patch; charset=UTF-8; name=pg_stat_statements_dealloc_fix_patch_2021_0319.patchDownload
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(&s->mutex);
+ s->stats.dealloc += 1;
+ SpinLockRelease(&s->mutex);
+ }
goto release_lock;
+ }
/*
* Reset global statistics for pg_stat_statements since all entries are
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.
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?