Re: Support reset of Shared objects statistics in "pg_stat_reset" function - Mailing list pgsql-hackers

On Sat, 7 Aug 2021 at 11:49, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Sat, 7 Aug 2021 at 00:13, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> >
> > On Fri, 6 Aug 2021 at 21:17, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Fri, Aug 6, 2021 at 8:53 PM Himanshu Upadhyaya
> > > <upadhyaya.himanshu@gmail.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
reasonfor 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
bezero)
 
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com



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