On Sat, 7 Aug 2021 at 00:13, Mahendra Singh Thalor <mahi6...@gmail.com> wrote: > > On Fri, 6 Aug 2021 at 21:17, Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Fri, Aug 6, 2021 at 8:53 PM Himanshu Upadhyaya > > <upadhyaya.himan...@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
all_shared_tables.sql
Description: Binary data