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

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=#


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