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

On Fri, 20 Aug 2021 at 07:37, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> wrote:
>
> On Fri, 20 Aug 2021 at 06:32, Sadhuprasad Patro <b(dot)sadhu(at)gmail(dot)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