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

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