Support reset of Shared objects statistics in "pg_stat_reset" function

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

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?

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:

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

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?

Thanks,
Himanshu

On Fri, Aug 6, 2021 at 5:40 PM Himanshu Upadhyaya <
upadhyaya.himanshu@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.

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

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

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

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

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

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

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 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 stats in
pgstat_recv_resetcounter for shared tables also because shared tables are
not much in counting so it will be good if we reset in one function only. I
will debug this part more and will see.

--
Thanks and Regards
Mahendra Singh Thalor
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 stats in pgstat_recv_resetcounter for shared tables also because shared tables are not much in counting so it will be good if 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 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 we can 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 stats for
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.

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 above comment with some additional
info, then it will be good.

--
Thanks and Regards
Mahendra Singh Thalor
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 we can 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 stats for 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 above comment 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 Sun, 22 Aug 2021 at 22:53, Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

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

--
Thanks and Regards
Mahendra Singh Thalor
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/&gt;

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/&gt;

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