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

On Fri, 6 Aug 2021 at 21:17, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Aug 6, 2021 at 8:53 PM Himanshu Upadhyaya
> <upadhyaya(dot)himanshu(at)gmail(dot)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