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

On Sat, 7 Aug 2021 at 11:49, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> wrote:
>
> 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

As of now, we are adding handling inside pg_stat_reset for shared
tables but I think we can add a new function with the name of
pg_stat_reset_shared_tables to reset stats for all the shared tables.
New function will give more clarity to the users also. We already have
a pg_stat_reset_shared(text) function for "archiver", "bgwriter", or
"wal".

Thoughts?

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

> 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

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