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

On Mon, 9 Aug 2021 at 06:56, Sadhuprasad Patro <b(dot)sadhu(at)gmail(dot)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