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

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
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.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com