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

Hi,

The call to “pg_stat_reset“ does reset all the statistics data for
tables belonging to the current database but does not take care of
shared tables e.g pg_attribute. Similarly to reset the statistics at
table level, the call to “pg_stat_reset_single_table_counters“ does
not take care of shared tables.

When we reset all the statistics using the call “pg_stat_reset”, the
postgres process internally makes calls to “
pgstat_recv_resetcounter“, which resets the statistics of all the
tables of the current database. But not resetting the statistics of
shared objects using database ID as 'InvalidOid'.

The same fix is made in the internal function
“pgstat_recv_resetsinglecounter“ to reset the statistics for the
shared table for the call "pg_stat_reset_single_table_counters".

--
thank u
SADHU PRASAD
EnterpriseDB: http://www.enterprisedb.com

On Fri, 6 Aug 2021 at 13:56, Sadhuprasad Patro <b.sadhu@gmail.com> wrote:
>
> Hi,
>
> The call to “pg_stat_reset“ does reset all the statistics data for
> tables belonging to the current database but does not take care of
> shared tables e.g pg_attribute. Similarly to reset the statistics at
> table level, the call to “pg_stat_reset_single_table_counters“ does
> not take care of shared tables.
>
> When we reset all the statistics using the call “pg_stat_reset”, the
> postgres process internally makes calls to “
> pgstat_recv_resetcounter“, which resets the statistics of all the
> tables of the current database. But not resetting the statistics of
> shared objects using database ID as 'InvalidOid'.
>
> The same fix is made in the internal function
> “pgstat_recv_resetsinglecounter“ to reset the statistics for the
> shared table for the call "pg_stat_reset_single_table_counters".
>

Hi Sadhu,
I was looking into the patch. I will look more in the coming days. I
have below comments.

1. Please can you share the test case to reproduce the issue and
please add the test case in patch.

2. diff --git a/src/backend/postmaster/pgstat.c
b/src/backend/postmaster/pgstat.c
index 56755cb92b..f272931276 100644
--- a/src/backend/postmaster/pgstat.c
+++ 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;

I think, by mistake, you removed one line in the patch.

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

On Fri, 6 Aug 2021 at 17:40, Himanshu Upadhyaya
<upadhyaya.himanshu@gmail.com> wrote:
>
> Hi Sadhu,
>
>
> > The call to “pg_stat_reset“ does reset all the statistics data for
> > tables belonging to the current database but does not take care of
> > shared tables e.g pg_attribute.
>
> pg_attribute is not a shared catalog, is the mentioned scenario is also applicable for few others tables?

Yes, I think, by mistake, Sadhu has mentioned pg_attribute.

With patch, I checked for pg_database and verified that we are resetting stats.

psql (15devel)
Type "help" for help.

postgres=# SELECT * FROM pg_statio_all_tables  where relid=1262;
 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();
 pg_stat_reset
---------------

(1 row)

postgres=# SELECT * FROM pg_statio_all_tables  where relid=1262;
 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 |
          0 |            0 |               0 |              0 |
      0 |             0
(1 row)

postgres=#


Shared tables are:
1. pg_database -- DatabaseRelationId 1262
2. pg_tablespcae -- TableSpaceRelationId 1213
3. pg_authid -- AuthIdRelationId 1260
4. pg_auth_members -- AuthMemRelationId 1261
5. pg_shdescription -- SharedDescriptionRelationId 2396
6. pg_shdepend -- SharedDependRelationId 1214
7. pg_shseclabel -- SharedSecLabelRelationId 3592
8. pg_db_role_setting -- DbRoleSettingRelationId 2694
9. pg_replication_origin -- ReplicationOriginRelationId 6000
10. pg_subscription -- SubscriptionRelationId 6100

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
>
> I have just tried it with-out your patch:
>
> postgres=# SELECT * FROM pg_statio_all_tables  where relid=1249;
>  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 
>
-------+------------+--------------+----------------+---------------+---------------+--------------+-----------------+----------------+----------------+---------------
>   1249 | pg_catalog | pg_attribute |             29 |           522 |             8 |          673 |
|               |                | 
> (1 row)
>
> postgres=# select pg_stat_reset();
>  pg_stat_reset
> ---------------
>
> (1 row)
>
> postgres=# SELECT * FROM pg_statio_all_tables  where relid=1249;
>  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 
>
-------+------------+--------------+----------------+---------------+---------------+--------------+-----------------+----------------+----------------+---------------
>   1249 | pg_catalog | pg_attribute |              0 |             0 |             0 |            0 |
|               |                | 
>
>
> We are able to reset the stats of pg_attibute without your patch.
>
> Thanks,
> Himanshu
>
> On Fri, Aug 6, 2021 at 1:56 PM Sadhuprasad Patro <b.sadhu@gmail.com> wrote:
>>
>> Hi,
>>
>> The call to “pg_stat_reset“ does reset all the statistics data for
>> tables belonging to the current database but does not take care of
>> shared tables e.g pg_attribute. Similarly to reset the statistics at
>> table level, the call to “pg_stat_reset_single_table_counters“ does
>> not take care of shared tables.
>>
>> When we reset all the statistics using the call “pg_stat_reset”, the
>> postgres process internally makes calls to “
>> pgstat_recv_resetcounter“, which resets the statistics of all the
>> tables of the current database. But not resetting the statistics of
>> shared objects using database ID as 'InvalidOid'.
>>
>> The same fix is made in the internal function
>> “pgstat_recv_resetsinglecounter“ to reset the statistics for the
>> shared table for the call "pg_stat_reset_single_table_counters".
>>
>> --
>> thank u
>> SADHU PRASAD
>> EnterpriseDB: http://www.enterprisedb.com



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

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.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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

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

Hi,

> > 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;
> >

Will Check the function "IsSharedRelation '' usage further and will
use it to fix the patch.
But I have not seen this function used every time as needed. No where
in the stat collector code.


> > 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)
> >

As we have given input as ObjectID of pg_database, it resets only the
counters of heap_blk_read & heap_blk_hit.
To reset other counters like index & toast table related, we need to
provide respective objectIDs and call the function again. Then it
resets everything.
This behavior is kept same as user tables local to the database.



> 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)

This function "pg_stat_reset_single_table_counters" works as per the
given table ID. To reset the index & toast table related counters, we
need to provide the respective ObjectIDs. (Refer example given for
pg_attribute).
So this works the same as a shared table and user tables local to the
current database.

> 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)
 

I did not find any existing test case for "pg_stat_reset", so I did
not add a new testcase for this issue..
I can add if needed.

-- 
Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

On Mon, 9 Aug 2021 at 06:56, Sadhuprasad Patro <b.sadhu@gmail.com> wrote:
>
> Hi,
>
> > > 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;
> > >
>
> Will Check the function "IsSharedRelation '' usage further and will
> use it to fix the patch.
> But I have not seen this function used every time as needed. No where
> in the stat collector code.
>
>
> > > 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)
> > >
>
> As we have given input as ObjectID of pg_database, it resets only the
> counters of heap_blk_read & heap_blk_hit.
> To reset other counters like index & toast table related, we need to
> provide respective objectIDs and call the function again. Then it
> resets everything.
> This behavior is kept same as user tables local to the database.

Thanks Sadhu for your explanation.

I also debugged this and understood the design.

-- 
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)
>
> This function "pg_stat_reset_single_table_counters" works as per the
> given table ID. To reset the index & toast table related counters, we
> need to provide the respective ObjectIDs. (Refer example given for
> pg_attribute).
> So this works the same as a shared table and user tables local to the
> current database.
>
> > 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
shouldbe zero)
 
>
> I did not find any existing test case for "pg_stat_reset", so I did
> not add a new testcase for this issue..
> I can add if needed.
>
> --
> Thanks & Regards
> SadhuPrasad
> EnterpriseDB: http://www.enterprisedb.com



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

> 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)
 

Adding a new test case for this looks difficult as results are not
consistent when the testcase added to automation. There may be some
possible delay as collector process has to reset the statistics in
background...
So Not adding any testcase and I think for the same reason there are
no existing test case for this stat reset.

Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

> 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?

In my opinion, it is better to extend the functionality of
"pg_stat_reset" call because a new function just to reset shared table
data may not be needed. Where we already have a reset shared function
"pg_stat_reset_shared" in place.

All of applicable comments are implemented in the patch below:


Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

On Tue, 10 Aug 2021 at 22:32, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Tue, 10 Aug 2021 at 21:53, Sadhuprasad Patro <b.sadhu@gmail.com> wrote:
> >
> > > 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?
> >
> > In my opinion, it is better to extend the functionality of
> > "pg_stat_reset" call because a new function just to reset shared table
> > data may not be needed. Where we already have a reset shared function
> > "pg_stat_reset_shared" in place.
> >
> > All of applicable comments are implemented in the patch below:
> >
>
> Hi Sadhu,
> I can see that you forgot to include "catalog.h" so I am getting below warning:
>>
>> pgstat.c: In function ‘pgstat_recv_resetsinglecounter’:
>> pgstat.c:5216:7: warning: implicit declaration of function ‘IsSharedRelation’; did you mean ‘InvalidRelation’?
[-Wimplicit-function-declaration]
>>   if (!IsSharedRelation(msg->m_objectid))
>>        ^~~~~~~~~~~~~~~~
>>        InvalidRelation
>
>
> 1) Please add the .h file.
> --- a/src/backend/postmaster/pgstat.c
> +++ b/src/backend/postmaster/pgstat.c
> @@ -38,6 +38,7 @@
>  #include "access/transam.h"
>  #include "access/twophase_rmgr.h"
>  #include "access/xact.h"
> +#include "catalog/catalog.h"
>  #include "catalog/partition.h"
>
> 2)
> @@ -1442,6 +1443,10 @@ pgstat_reset_counters(void)
>         pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_RESETCOUNTER);
>         msg.m_databaseid = MyDatabaseId;
>         pgstat_send(&msg, sizeof(msg));
> +
> +       /* Reset the stat counters for Shared tables also. */
> +       msg.m_databaseid = InvalidOid;
> +       pgstat_send(&msg, sizeof(msg));
>
> I will look into this part again. If pgstat_send forks a new process, then I think, it will be better if we can reset
statsin pgstat_recv_resetcounter for shared tables also because shared tables are not much in counting so it will be
goodif we reset in one function only. I will debug this part more and will see. 
>

I checked this and found that we already have one process "stats
collector" and in v02 patch, we are sending requests to collect stats
two times. I don't know how costly one call is but I feel that we can
avoid the 2nd call by keeping handling of the shared tables into
pgstat_recv_resetcounter.

Thoughts?

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

On Tue, Aug 10, 2021 at 10:49 PM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:

> I checked this and found that we already have one process "stats
> collector" and in v02 patch, we are sending requests to collect stats
> two times. I don't know how costly one call is but I feel that we can
> avoid the 2nd call by keeping handling of the shared tables into
> pgstat_recv_resetcounter.
>

I think let's not make the stat collector aware of what we want to
achieve, so the stat collector will only reset for what we have asked
for and let the caller or the signal sender decide what they want to
do.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, 11 Aug 2021 at 09:17, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Aug 10, 2021 at 10:49 PM Mahendra Singh Thalor
> <mahi6run@gmail.com> wrote:
>
> > I checked this and found that we already have one process "stats
> > collector" and in v02 patch, we are sending requests to collect stats
> > two times. I don't know how costly one call is but I feel that we can
> > avoid the 2nd call by keeping handling of the shared tables into
> > pgstat_recv_resetcounter.
> >
>
> I think let's not make the stat collector aware of what we want to
> achieve, so the stat collector will only reset for what we have asked
> for and let the caller or the signal sender decide what they want to
> do.
>
Thanks Dilip for your opinion.

If we want to use pgstat_recv_resetcounter with invalid database oid,
then we should update all the comments of pgstat_recv_resetcounter
function because we are calling this function with both valid and
invalid database oid.
If we are passing invalid database oid, it means we want to reset
stats for shared tables.

1)
    /*
     * Lookup the database in the hashtable.  Nothing to do if not
there.
     */
    dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
We should update the above comment and if m_databaseid is invalid,
then we should always get dbentry.

Assert (msg->m_databaseid == 0 && dbentry ) and some more sanity checks.

2)
    /*
     * We simply throw away all the database's table entries by recreating a
     * new hash table for them.
     */
I think we should update this also.

3)
*  Reset the statistics for the specified database.
This should be updated.

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

On Wed, Aug 11, 2021 at 10:30 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
>
> On Wed, 11 Aug 2021 at 09:17, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Aug 10, 2021 at 10:49 PM Mahendra Singh Thalor
> > <mahi6run@gmail.com> wrote:
> >
> > > I checked this and found that we already have one process "stats
> > > collector" and in v02 patch, we are sending requests to collect stats
> > > two times. I don't know how costly one call is but I feel that we can
> > > avoid the 2nd call by keeping handling of the shared tables into
> > > pgstat_recv_resetcounter.
> > >
> >
> > I think let's not make the stat collector aware of what we want to
> > achieve, so the stat collector will only reset for what we have asked
> > for and let the caller or the signal sender decide what they want to
> > do.

If we do support resetting the stats for shared tables in
'pg_stat_reset', which is for DB level,
then the stats of shared tables will be reseted in other instances as
well, which seems to be not correct.
So we need to provide some way to reset the stats for shared tables to
customers.

In the latest patch, we have supported only through the
pgstat_recv_resetsinglecounter function to reset stats for shared
tables.

Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

> If we do support resetting the stats for shared tables in
> 'pg_stat_reset', which is for DB level,
> then the stats of shared tables will be reseted in other instances as
> well, which seems to be not correct.
> So we need to provide some way to reset the stats for shared tables to
> customers.
>
> In the latest patch, we have supported only through the
> pgstat_recv_resetsinglecounter function to reset stats for shared
> tables.

Final patch attached after some corrections..

Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

On Fri, 20 Aug 2021 at 07:37, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Fri, 20 Aug 2021 at 06:32, Sadhuprasad Patro <b.sadhu@gmail.com> wrote:
> >
> > > If we do support resetting the stats for shared tables in
> > > 'pg_stat_reset', which is for DB level,
> > > then the stats of shared tables will be reseted in other instances as
> > > well, which seems to be not correct.
> > > So we need to provide some way to reset the stats for shared tables to
> > > customers.
>
> JFI, I am briefly explaining here.
>
> In an offline discussion with Robert Haas, we discussed all the review comments with him and then he suggested that
wecan consider this as a small bug in pgstat_recv_resetsinglecounter because when we are giving shared table oid to
pgstat_recv_resetsinglecounter,then it is not doing anything so we can add handling for this so that users can reset
statsfor shared table also and there is no need to reset stats from pg_reset_stats for all the shard tables. 
> So based on that, we decided that we should add handling only in pgstat_recv_resetsinglecounter
> Thanks Robert Hass for your opinion.
>
> > >
> > > In the latest patch, we have supported only through the
> > > pgstat_recv_resetsinglecounter function to reset stats for shared
> > > tables.
> >
> > Final patch attached after some corrections..
>
> Thanks Sadhu for the updated patch.
>
> I reviewed v4* patch. Below are some of my some review comments.
>
> 1)
>          Resets statistics for a single table or index in the current database
> -        to zero.
> +        to zero. The input can be a shared table also
>
> I think, above comment should be modified. Maybe, we can modify it as "If input is a shared oid(table or index or
toast),then resets statistics for a single shared entry to zero. 
>
> 2)
>  #include "catalog/pg_proc.h"
> +#include "catalog/catalog.h"
>  #include "common/ip.h"
> Header file should be in alphabetical order.
>
> 3)
> -       dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
> +       if (!IsSharedRelation(msg->m_objectid))
> +               dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
> +       else
> +               dbentry = pgstat_get_db_entry(InvalidOid, false);
>
> We should add some comments before this change.

Above changes can be replaced with below code(Either 1 or 2):

1)
@@ -5143,6 +5143,13 @@
pgstat_recv_resetsinglecounter(PgStat_MsgResetsinglecounter *msg, int
len)
 {
        PgStat_StatDBEntry *dbentry;

+       /*
+        * If oid refers to shared object, then set m_databaseid as invalid so
+        * that we can reset stats for shared object also.
+        */
+       if (IsSharedRelation(msg->m_objectid))
+               msg->m_databaseid = InvalidOid;
+
        dbentry = pgstat_get_db_entry(msg->m_databaseid, false);

2)
@@ -5143,7 +5143,11 @@
pgstat_recv_resetsinglecounter(PgStat_MsgResetsinglecounter *msg, int
len)
 {
        PgStat_StatDBEntry *dbentry;

-       dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
+       /* some comment */
+       if (IsSharedRelation(msg->m_objectid))
+               dbentry = pgstat_get_db_entry(InvalidOid, false);
+       else
+               dbentry = pgstat_get_db_entry(msg->m_databaseid, false);

In 'if' condition, we can check shared and then if object is shared,
then pass 'invalidOid' in 'if' case only so that it will give more
readability and code will be cleaner.

Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
>
> 4)
>  /* ----------
>   * pgstat_recv_resetsinglecounter() -
>   *
> - *     Reset a statistics for a single object
> + *     Reset a statistics for a single object, which may also be a shared
> + *     table.
>
> table should be replaced with 'object' as we have table, index, toast for shared tables and if we can modify the
abovecomment with some additional info, then it will be good. 
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com



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

On 2021/08/20 11:07, Mahendra Singh Thalor wrote:
> 1)
>           Resets statistics for a single table or index in the current database
> -        to zero.
> +        to zero. The input can be a shared table also
> 
> I think, above comment should be modified. Maybe, we can modify it as "If input is a shared oid(table or index or
toast),then resets statistics for a single shared entry to zero.
 

I'm not sure if ordinary users can understand what "shared oid" means. Instead,
what about "Resets statistics for a single relation in the current database or
shared across all databases in the cluster to zero."?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

> 3)
> -       dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
> +       if (!IsSharedRelation(msg->m_objectid))
> +               dbentry = pgstat_get_db_entry(msg->m_databaseid, false);
> +       else
> +               dbentry = pgstat_get_db_entry(InvalidOid, false);
>
> We should add some comments before this change.

In my opinion, the comments added above the function
"pgstat_recv_resetsinglecounter" and the function call
"IsSharedRelation" added are self explanatory. If we add anything
more, it will be a duplication.
So No need to add any more comments here.


Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

> On 2021/08/20 11:07, Mahendra Singh Thalor wrote:
> > 1)
> >           Resets statistics for a single table or index in the current database
> > -        to zero.
> > +        to zero. The input can be a shared table also
> >
> > I think, above comment should be modified. Maybe, we can modify it as "If input is a shared oid(table or index or
toast),then resets statistics for a single shared entry to zero.
 
>
> I'm not sure if ordinary users can understand what "shared oid" means. Instead,
> what about "Resets statistics for a single relation in the current database or
> shared across all databases in the cluster to zero."?
>

Thank you for the review here. As per the comments, attached the
latest patch here...


Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

On 2021/08/24 13:07, Mahendra Singh Thalor wrote:
> Thanks Sadhu for the updated patch.
> 
> Patch looks good to me and I don't have any more comments.
> 
> I marked this patch as 'ready for committer'.
> https://commitfest.postgresql.org/34/3282/ <https://commitfest.postgresql.org/34/3282/>

Attached is the updated version of the patch. In this patch, I updated
the description for pg_stat_reset_single_table_counters() in pg_proc.dat.
Barring any objection, I will commit this patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Sep 1, 2021 at 5:31 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2021/08/24 13:07, Mahendra Singh Thalor wrote:
> > Thanks Sadhu for the updated patch.
> >
> > Patch looks good to me and I don't have any more comments.
> >
> > I marked this patch as 'ready for committer'.
> > https://commitfest.postgresql.org/34/3282/ <https://commitfest.postgresql.org/34/3282/>
>
> Attached is the updated version of the patch. In this patch, I updated
> the description for pg_stat_reset_single_table_counters() in pg_proc.dat.
> Barring any objection, I will commit this patch.

Hi Fujii,

Yes this update is fine.. Please commit this patch...

Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

On 2021/09/02 13:36, Sadhuprasad Patro wrote:
> Yes this update is fine.. Please commit this patch...

Yeah, pushed. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION