Re: New SQL counter statistics view (pg_stat_sql)

On Wed, Jan 18, 2017 at 10:45 AM, Haribabu Kommi
<kommi(dot)haribabu(at)gmail(dot)com> wrote:
> 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.

Okay, As per your explaination, and Robert also pointed out that it
can be a feature for someone and bug for others. I would have
preferred it shows actual SQL statement. But I have no objection to
what you are doing.
>
>
>>
>> + /* 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.

Okay..
>
>>
>> ------
>> 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.

Okay..
>
>>>+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.

Overall patch looks fine to me and marking it "ready for committer".

There is two design decision, which I leave it for committer's decision.

1. EXECUTE statement should show only as EXECUTE count, not the
actual SQL query.
2. should we count statement execute from Functions or only statement
what is executed directly by the user as it's doing now?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com