Re: Support reset of Shared objects statistics in "pg_stat_reset" function

On Sat, 7 Aug 2021 at 00:13, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
wrote:
>
> On Fri, 6 Aug 2021 at 21:17, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Fri, Aug 6, 2021 at 8:53 PM Himanshu Upadhyaya
> > <upadhyaya(dot)himanshu(at)gmail(dot)com> wrote:
> > >
> > > Hi Sadhu,
> > >
> > > Patch working as expected with shared tables, just one Minor comment
on the patch.
> > > + if (!dbentry)
> > > + return;
> > > +
> > > + /*
> > > + * We simply throw away all the shared table entries by
recreating new
> > > + * hash table for them.
> > > + */
> > > + if (dbentry->tables != NULL)
> > > + hash_destroy(dbentry->tables);
> > > + if (dbentry->functions != NULL)
> > > + hash_destroy(dbentry->functions);
> > > +
> > > + dbentry->tables = NULL;
> > > + dbentry->functions = NULL;
> > > +
> > > + /*
> > > + * This creates empty hash tables for tables and functions.
> > > + */
> > > + reset_dbentry_counters(dbentry);
> > >
> > > We already have the above code for non-shared tables, can we restrict
duplicate code?
> > > one solution I can think of, if we can have a list with two elements
and iterate each element with
> > > these common steps?
> >
> > Another idea could be that instead of putting this logic in
> > pgstat_recv_resetcounter(), we can have this logic in pg_stat_reset()
> > or maybe in pgstat_reset_counters(). So now
> > pgstat_recv_resetcounter() logic remain the same and I think that
> > logic is much cleaner i.e. whatever dobid it got in the message it
> > will reset stat for that dboid.
> >
> > So now, if it depends upon the logic of the callers that what they
> > want to do so in this case pgstat_recv_resetcounter(), can send two
> > messages one for MyDatabaseOid which it is already doing, and another
> > message for the InvalidOid.
> >
> Hi,
> I reviewed patch and please find my review comments below:
>
> 1)
> In pgstat_recv_resetcounter, first we are checking for m_databaseid.
>
> +++ b/src/backend/postmaster/pgstat.c
> @@ -5092,7 +5092,6 @@ pgstat_recv_resetcounter(PgStat_MsgResetcounter
> *msg, int len)
> * Lookup the database in the hashtable. Nothing to do if not there.
> */
> dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
>
> if (!dbentry)
> return;
> If we don't find any dbentry, then we are returning but I think we
> should try to reset stats for shared tables. I may be wrong because I
> haven't tested this.
>
> 2)
> +
> + /*
> + * Lookup for the shared tables also to reset the stats
> + */
> + dbentry = pgstat_get_db_entry(InvalidOid, false);
> + if (!dbentry)
> + return;
>
> I think, always we should get dbentry for shared tables so we can add
> assert here.
>
> + Assert (dbentry);
>
> 3)
> pgstat_recv_resetsinglecounter(PgStat_MsgResetsinglecounter *msg, int
len)
> {
> PgStat_StatDBEntry *dbentry;
> + bool found;
>
> dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
>
> @@ -5168,13 +5192,41 @@
> pgstat_recv_resetsinglecounter(PgStat_MsgResetsinglecounter *msg, int
> len)
> /* Set the reset timestamp for the whole database */
> dbentry->stat_reset_timestamp = GetCurrentTimestamp();
>
> - /* Remove object if it exists, ignore it if not */
> + /* Remove object if it exists, if not then may be it's a shared
table */
> if (msg->m_resettype == RESET_TABLE)
> + {
> (void) hash_search(dbentry->tables, (void *) &(msg->m_objectid),
> - HASH_REMOVE, NULL);
> + HASH_REMOVE, &found);
> + if (!found)
> + {
> + /* If we didn't find it, maybe it's a shared table */
> + dbentry = pgstat_get_db_entry(InvalidOid, false);
> + if (dbentry)
> + {
> + /* Set the reset timestamp for the whole database */
> + dbentry->stat_reset_timestamp = GetCurrentTimestamp();
> + (void) hash_search(dbentry->tables, (void *)
> &(msg->m_objectid),
> + HASH_REMOVE, NULL);
> + }
> + }
> + }
> else if (msg->m_resettype == RESET_FUNCTION)
> + {
> (void) hash_search(dbentry->functions, (void *)
&(msg->m_objectid),
> - HASH_REMOVE, NULL);
> + HASH_REMOVE, &found);
> + if (!found)
> + {
> + /* If we didn't find it, maybe it's a shared table */
> + dbentry = pgstat_get_db_entry(InvalidOid, false);
> + if (dbentry)
> + {
> + /* Set the reset timestamp for the whole database */
> + dbentry->stat_reset_timestamp = GetCurrentTimestamp();
> + (void) hash_search(dbentry->functions, (void *)
> &(msg->m_objectid),
> + HASH_REMOVE, NULL);
> + }
> + }
> + }
> }
>
> Above code can be replaced with:
> @@ -5160,7 +5162,10 @@
> pgstat_recv_resetsinglecounter(PgStat_MsgResetsinglecounter *msg, int
> len)
> {
> PgStat_StatDBEntry *dbentry;
>
> - dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
> + if (IsSharedRelation(msg->m_objectid))
> + dbentry = pgstat_get_db_entry(InvalidOid, false);
> + else
> + dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
>
> if (!dbentry)
> return;
>
> 4) In the attached patch, I made one common function to reset dbentry
> and this common function fixes my 1st comment also.
>
> 5) pg_stat_reset_single_table_counters is not resetting all the
> columns for pg_database.
> Ex:
> postgres=# SELECT * FROM pg_statio_all_tables where relid =
> 'pg_database'::regclass::oid;
> relid | schemaname | relname | heap_blks_read | heap_blks_hit |
> idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
> tidx_blks_read | tidx_blks_hit
>
-------+------------+-------------+----------------+---------------+---------------+--------------+-----------------+----------------+----------------+---------------
> 1262 | pg_catalog | pg_database | 1 | 2 |
> 2 | 0 | 0 | 0 |
> 0 | 0
> (1 row)
>
> postgres=# select
> pg_stat_reset_single_table_counters('pg_database'::regclass::oid);
> pg_stat_reset_single_table_counters
> -------------------------------------
>
> (1 row)
>
> postgres=# SELECT * FROM pg_statio_all_tables where relid =
> 'pg_database'::regclass::oid;
> relid | schemaname | relname | heap_blks_read | heap_blks_hit |
> idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
> tidx_blks_read | tidx_blks_hit
>
-------+------------+-------------+----------------+---------------+---------------+--------------+-----------------+----------------+----------------+---------------
> 1262 | pg_catalog | pg_database | 0 | 0 |
> 2 | 0 | 0 | 0 |
> 0 | 0
> (1 row)
>
> postgres=#

I have some more review comments.

*1)* We should update the document for both the functions. May be, we can
update like:

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 74a58a916c..232764a5dd 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5056,7 +5056,8 @@ SELECT pid, wait_event_type, wait_event FROM
pg_stat_activity WHERE wait_event i
<returnvalue>void</returnvalue>
</para>
<para>
- Resets all statistics counters for the current database to zero.
+ Resets all statistics counters for the current database to zero and
+ reset for shared tables also.
</para>
<para>
This function is restricted to superusers by default, but other
users
@@ -5098,6 +5099,8 @@ SELECT pid, wait_event_type, wait_event FROM
pg_stat_activity WHERE wait_event i
<para>
Resets statistics for a single table or index in the current
database
to zero.
+ NOTE: if we pass shared table oid, then restes statistics for
shared
+ table also.
</para>
<para>
This function is restricted to superusers by default, but other
users
*2) *I think, pg_stat_reset_single_table_counters is not working properly
for shared tables so we should find out the reason for this.

*Ex: (I tested for pg_database)*
postgres=# SELECT * FROM pg_statio_all_tables where relid =
'pg_database'::regclass::oid;
relid | schemaname | relname | heap_blks_read | heap_blks_hit |
idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
tidx_blks_read | tidx_blks_hit
-------+------------+-------------+----------------+---------------+---------------+--------------+-----------------+----------------+----------------+---------------
1262 | pg_catalog | pg_database | 1 | 2 |
2 | 0 | 0 | 0 | 0
| 0
(1 row)
postgres=#
postgres=# select
pg_stat_reset_single_table_counters('pg_database'::regclass::oid);
pg_stat_reset_single_table_counters
-------------------------------------

(1 row)
postgres=# SELECT * FROM pg_statio_all_tables where relid =
'pg_database'::regclass::oid;
relid | schemaname | relname | heap_blks_read | heap_blks_hit |
idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
tidx_blks_read | tidx_blks_hit
-------+------------+-------------+----------------+---------------+---------------+--------------+-----------------+----------------+----------------+---------------
1262 | pg_catalog | pg_database | 0 | 0 |
2 | 0 | 0 | 0 | 0
| 0
(1 row)

3) I am attaching a .sql file. We can add similar types of test cases for
shared tables.
Ex: first reset stats for all shared tables using pg_stat_reset and then
check stats for all shared tables(all should be zero)

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com