Re: New SQL counter statistics view (pg_stat_sql)

On 2017/01/18 14:15, Haribabu Kommi wrote:
>
>
> On Sat, Jan 14, 2017 at 2:58 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com
> <mailto:dilipbalaut(at)gmail(dot)com>> wrote:
>
> On Wed, Jan 11, 2017 at 7:47 PM, Dilip Kumar
> <dilipbalaut(at)gmail(dot)com <mailto:dilipbalaut(at)gmail(dot)com>> wrote:
> > +void
> > +pgstat_count_sqlstmt(const char *commandTag)
> > +{
> > + PgStat_SqlstmtEntry *htabent;
> > + bool found;
> > +
> > + if (!pgstat_track_sql)
> > + return
> >
> > Callers of pgstat_count_sqlstmt are always ensuring that
> > pgstat_track_sql is true, seems it's redundant here.
>
> I have done testing of the feature, it's mostly working as per the
> expectation.
>
>
> Thanks for the review and test.
>
> The use case for this view is to find out the number of different SQL
> statements
> that are getting executed successfully on an instance by the
> application/user.
>
> I have few comments/questions.
>
> --------
> If you execute the query with EXECUTE then commandTag will be EXECUTE
> that way we will not show the actual query type, I mean all the
> statements will get the common tag "EXECUTE".
>
> You can try this.
> PREPARE fooplan (int) AS INSERT INTO t VALUES($1);
> EXECUTE fooplan(1);
>
> ------
>
>
> Yes, that's correct. Currently the count is incremented based on the
> base command,
> because of this reason, the EXECUTE is increased, instead of the
> actual operation.
>
> + /* Destroy the SQL statement counter stats HashTable */
> + hash_destroy(pgstat_sql);
> +
> + /* Create SQL statement counter Stats HashTable */
>
> I think in the patch we have used mixed of "Stats HashTable" and
> "stats HashTable", I think better
> to be consistent throughout the patch. Check other similar instances.
>
> ----------------
>
>
> Corrected.
>
>
> @@ -1102,6 +1102,12 @@ exec_simple_query(const char *query_string)
>
> PortalDrop(portal, false);
>
> + /*
> + * Count SQL statement for pg_stat_sql view
> + */
> + if (pgstat_track_sql)
> + pgstat_count_sqlstmt(commandTag);
>
> We are only counting the successful SQL statement, Is that
> intentional?
>
>
> Yes, I thought of counting the successful SQL operations that changed the
> database over a period of time. I am not sure whether there will be many
> failure operations that can occur on an instance to be counted.
>
> ------
> I have noticed that pgstat_count_sqlstmt is called from
> exec_simple_query and exec_execute_message. Don't we want to track the
> statement executed from functions?
> -------
>
> The view is currently designed to count user/application initiated SQL
> statements,
> but not the internal SQL queries that are getting executed from
> functions and etc.
>
> >>+void
> >>+pgstat_count_sqlstmt(const char *commandTag)
> >>+{
> >>+ PgStat_SqlstmtEntry *htabent;
> >>+ bool found;
> >>+
> >>+ if (!pgstat_track_sql)
> >>+ return
> >
> >Callers of pgstat_count_sqlstmt are always ensuring that
> >pgstat_track_sql is true, seems it's redundant here.
>
> Removed the check in pgstat_count_sqlstmt statement and add it
> exec_execute_message
> function where the check is missed.
>
> Updated patch attached.
I have reviewed the patch. All the test cases works fine.

Regards,
Vinayak Pokale
NTT Open Source Software Center