Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Thu, Apr 6, 2017 at 5:17 AM, Andres Freund  wrote:

> Hi,
>
>
> I'm somewhat inclined to think that this really would be better done in
> an extension like pg_stat_statements.
>

Thanks for the review.



>
> > +}
> > +
> > +/*
> > + * Count SQL statement for pg_stat_sql view
> > + */
> > +void
> > +pgstat_count_sqlstmt(const char *commandTag)
> > +{
> > + PgStat_SqlstmtEntry *htabent;
> > + boolfound;
> > +
> > + if (!pgstat_backend_sql)
> > + {
> > + /* First time through - initialize SQL statement stat
> table */
> > + HASHCTL hash_ctl;
> > +
> > + memset(_ctl, 0, sizeof(hash_ctl));
> > + hash_ctl.keysize = NAMEDATALEN;
> > + hash_ctl.entrysize = sizeof(PgStat_SqlstmtEntry);
> > + pgstat_backend_sql = hash_create("SQL statement stat
> entries",
> > +
>   PGSTAT_SQLSTMT_HASH_SIZE,
> > +
>   _ctl,
> > +
>   HASH_ELEM | HASH_BLOBS);
> > + }
> > +
> > + /* Get the stats entry for this SQL statement, create if necessary
> */
> > + htabent = hash_search(pgstat_backend_sql, commandTag,
> > +   HASH_ENTER, );
> > + if (!found)
> > + htabent->count = 1;
> > + else
> > + htabent->count++;
> > +}
>
>
> That's not really something in this patch, but a lot of this would be
> better if we didn't internally have command tags as strings, but as an
> enum.  We should really only convert to a string with needed.  That
> we're doing string comparisons on Portal->commandTag is just plain bad.
>
> If so, we could also make this whole patch a lot cheaper - have a fixed
> size array that has an entry for every possible tag (possibly even in
> shared memory, and then use atomics there).
>

During the development, I thought of using an array of all command tags
and update the counters using the tag name, but not like the enum values.
Now I have to create a mapping array with enums and tag names for easier
counter updates. The problem in this approach is, whenever any addition
of new commands, this mapping array needs to be updated with both
new enum and new tag name, whereas with hash table approach, it works
future command additions also, but there is some performance overhead
compared to the array approach.

I will modify the patch to use the array approach and provide it to the next
commitfest.



> > +++ b/src/backend/tcop/postgres.c
> > @@ -1109,6 +1109,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);
> > +
>
> Isn't doing this at the message level quite wrong?  What about
> statements executed from functions and such?  Shouldn't this integrate
> at the executor level instead?
>

Currently the patch is designed to find out only the user executed
statements
that are successfully finished, (no internal statements that are executed
from
functions and etc).

The same question was asked by dilip also in earlier mails, may be now it is
the time that we can decide the approach of statement counts.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Andres Freund]

Hi,


I'm somewhat inclined to think that this really would be better done in
an extension like pg_stat_statements.


On 2017-03-08 14:39:23 +1100, Haribabu Kommi wrote:
> On Wed, Feb 1, 2017 at 3:13 PM, Michael Paquier 

> + 
> +  track_sql (boolean)
> +  
> +   track_sql configuration parameter
> +  
> +  
> +  
> +   
> +Enables collection of different SQL statement statistics that are
> +executed on the instance. This parameter is off by default. Only
> +superusers can change this setting.
> +   
> +  
> + 
> +

I don't like this name much, seems a bit too generic to
me. 'track_activities', 'track_io_timings' are at least a bit clearer.
How about track_statement_statistics + corresponding view/function renaming?


> +  
> +   pg_stat_sql View
> +   
> +
> +
> +  Column
> +  Type
> +  Description
> + 
> +
> +
> +   
> +
> + tag
> + text
> + Name of the SQL statement
> +

It's not really the "name" of a statement. Internally and IIRC elsewhere
in the docs we describe something like this as tag?


> +/* --
> + * pgstat_send_sqlstmt(void)
> + *
> + *   Send SQL statement statistics to the collector
> + * --
> + */
> +static void
> +pgstat_send_sqlstmt(void)
> +{
> + PgStat_MsgSqlstmt msg;
> + PgStat_SqlstmtEntry *entry;
> + HASH_SEQ_STATUS hstat;
> +
> + if (pgstat_backend_sql == NULL)
> + return;
> +
> + pgstat_setheader(_hdr, PGSTAT_MTYPE_SQLSTMT);
> + msg.m_nentries = 0;
> +
> + hash_seq_init(, pgstat_backend_sql);
> + while ((entry = (PgStat_SqlstmtEntry *) hash_seq_search()) != 
> NULL)
> + {
> + PgStat_SqlstmtEntry *m_ent;
> +
> + /* Skip it if no counts accumulated since last time */
> + if (entry->count == 0)
> + continue;
> +
> + /* need to convert format of time accumulators */
> + m_ent = _entry[msg.m_nentries];
> + memcpy(m_ent, entry, sizeof(PgStat_SqlstmtEntry));
> +
> + if (++msg.m_nentries >= PGSTAT_NUM_SQLSTMTS)
> + {
> + pgstat_send(, offsetof(PgStat_MsgSqlstmt, 
> m_entry[0]) +
> + msg.m_nentries * 
> sizeof(PgStat_SqlstmtEntry));
> + msg.m_nentries = 0;
> + }
> +
> + /* reset the entry's count */
> + entry->count = 0;
> + }
> +
> + if (msg.m_nentries > 0)
> + pgstat_send(, offsetof(PgStat_MsgSqlstmt, m_entry[0]) +
> + msg.m_nentries * 
> sizeof(PgStat_SqlstmtEntry));

Hm. So pgstat_backend_sql is never deleted, which'll mean that if a
backend lives long we'll continually iterate over a lot of 0 entries.  I
think performance evaluation of this feature should take that into
account.


> +}
> +
> +/*
> + * Count SQL statement for pg_stat_sql view
> + */
> +void
> +pgstat_count_sqlstmt(const char *commandTag)
> +{
> + PgStat_SqlstmtEntry *htabent;
> + boolfound;
> +
> + if (!pgstat_backend_sql)
> + {
> + /* First time through - initialize SQL statement stat table */
> + HASHCTL hash_ctl;
> +
> + memset(_ctl, 0, sizeof(hash_ctl));
> + hash_ctl.keysize = NAMEDATALEN;
> + hash_ctl.entrysize = sizeof(PgStat_SqlstmtEntry);
> + pgstat_backend_sql = hash_create("SQL statement stat entries",
> + 
>  PGSTAT_SQLSTMT_HASH_SIZE,
> + 
>  _ctl,
> + 
>  HASH_ELEM | HASH_BLOBS);
> + }
> +
> + /* Get the stats entry for this SQL statement, create if necessary */
> + htabent = hash_search(pgstat_backend_sql, commandTag,
> +   HASH_ENTER, );
> + if (!found)
> + htabent->count = 1;
> + else
> + htabent->count++;
> +}


That's not really something in this patch, but a lot of this would be
better if we didn't internally have command tags as strings, but as an
enum.  We should really only convert to a string with needed.  That
we're doing string comparisons on Portal->commandTag is just plain bad.

If so, we could also make this whole patch a lot cheaper - have a fixed
size array that has an entry for every possible tag (possibly even in
shared memory, and then use atomics there).

> +++ b/src/backend/tcop/postgres.c
> @@ -1109,6 +1109,12 @@ exec_simple_query(const char *query_string)
>  
>   PortalDrop(portal, false);
>  
> + /*
> +  * Count SQL statement for pg_stat_sql view
> +  */
> + if (pgstat_track_sql)
> + 

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Wed, Feb 1, 2017 at 3:13 PM, Michael Paquier 
wrote:

> On Fri, Jan 27, 2017 at 10:26 AM, Haribabu Kommi
>  wrote:
> > Thanks for the review.
> > Let's wait for the committer's opinion.
>
> I have moved this patch to CF 2017-03 to wait for this to happen.
>

Attached a rebased patch to resolve the OID conflict.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_sql_row_mode_5.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Michael Paquier]

On Fri, Jan 27, 2017 at 10:26 AM, Haribabu Kommi
 wrote:
> Thanks for the review.
> Let's wait for the committer's opinion.

I have moved this patch to CF 2017-03 to wait for this to happen.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Tue, Jan 24, 2017 at 3:59 PM, Dilip Kumar  wrote:

>
> 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?


Thanks for the review.
Let's wait for the committer's opinion.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Dilip Kumar]

On Wed, Jan 18, 2017 at 10:45 AM, Haribabu Kommi
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[vinayak]

On 2017/01/18 14:15, Haribabu Kommi wrote:



On Sat, Jan 14, 2017 at 2:58 AM, Dilip Kumar > wrote:


On Wed, Jan 11, 2017 at 7:47 PM, Dilip Kumar
> 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

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Sat, Jan 14, 2017 at 2:58 AM, Dilip Kumar  wrote:

> On Wed, Jan 11, 2017 at 7:47 PM, Dilip Kumar 
> 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.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_sql_row_mode_4.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Robert Haas]

On Fri, Jan 13, 2017 at 10:58 AM, Dilip Kumar  wrote:
> 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".

Somebody might say that's a feature, not a bug.  But then again,
someone else might say the opposite.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Michael Paquier]

On Sat, Jan 14, 2017 at 12:58 AM, Dilip Kumar  wrote:
> On Wed, Jan 11, 2017 at 7:47 PM, Dilip Kumar  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.
>
> 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);
>
> --
>
> + /* 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.
>
> 
>
> @@ -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?
>
> --
> 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 latest patch available has rotten, could you rebase please?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Dilip Kumar]

On Wed, Jan 11, 2017 at 7:47 PM, Dilip Kumar  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.

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);

--

+ /* 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.



@@ -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?

--
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?
---

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Robert Haas]

On Wed, Jan 11, 2017 at 9:17 AM, Dilip Kumar  wrote:
> On Mon, Dec 5, 2016 at 8:01 AM, Haribabu Kommi  
> wrote:
>> Moved to next CF with "needs review" status.
>
> I have started reviewing the patch, Some initial comments.
>
> $ git apply ../pg_stat_sql_row_mode_2.patch
> error: patch failed: src/include/catalog/pg_proc.h:2893
> error: src/include/catalog/pg_proc.h: patch does not apply

I suggest using 'patch' directly.  'git apply' is far too picky.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Dilip Kumar]

On Mon, Dec 5, 2016 at 8:01 AM, Haribabu Kommi  wrote:
> Moved to next CF with "needs review" status.

I have started reviewing the patch, Some initial comments.

$ git apply ../pg_stat_sql_row_mode_2.patch
error: patch failed: src/include/catalog/pg_proc.h:2893
error: src/include/catalog/pg_proc.h: patch does not apply

Patch is not applying on the head, I think it needs to be rebased.

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

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Tue, Nov 22, 2016 at 5:57 PM, Haribabu Kommi 
wrote:

> Hi Vinayak,
>
> This is a gentle reminder.
>
> you assigned as reviewer to the current patch in the 11-2016 commitfest.
> If you have any more review comments / performance results regarding the
> approach of the patch, please share it. Otherwise, you can mark the patch
> as "Ready for committer" for committer feedback. This will help us in
> smoother
> operation of the commitfest.
>

Moved to next CF with "needs review" status.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

Hi Vinayak,

This is a gentle reminder.

you assigned as reviewer to the current patch in the 11-2016 commitfest.
If you have any more review comments / performance results regarding the
approach of the patch, please share it. Otherwise, you can mark the patch
as "Ready for committer" for committer feedback. This will help us in
smoother
operation of the commitfest.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Wed, Oct 19, 2016 at 5:11 AM, Robert Haas  wrote:

> On Thu, Sep 29, 2016 at 1:45 AM, Haribabu Kommi
>  wrote:
> > Currently, The SQL stats is a fixed size counter to track the all the
> ALTER
> > cases as single counter. So while sending the stats from the backend to
> > stats collector at the end of the transaction, the cost is same, because
> of
> > it's fixed size. This approach adds overhead to send and read the stats
> > is minimal.
> >
> > With the following approach, I feel it is possible to support the
> counter at
> > command tag level.
> >
> > Add a Global and local Hash to keep track of the counters by using the
> > command tag as the key, this hash table increases dynamically whenever
> > a new type of SQL command gets executed. The Local Hash data is passed
> > to stats collector whenever the transaction gets committed.
> >
> > The problem I am thinking is that, Sending data from Hash and populating
> > the Hash from stats file for all the command tags adds some overhead.
>
> Yeah, I'm not very excited about that overhead.  This seems useful as
> far as it goes, but I don't really want to incur measurable overhead
> when it's in use.  Having a hash table rather than a fixed array of
> slots means that you have to pass this through the stats collector
> rather than updating shared memory directly, which is fairly heavy
> weight.  If each backend could have its own copy of the slot array and
> just update that, and readers added up the values across the whole
> array, this could be done without any locking at all, and it would
> generally be much lighter-weight than this approach.


Using limited information like combining all ALTER XXX into Alter counter,
the fixed array logic works fine without having any problems. But people
are suggesting to provide more details like ALTER VIEW and etc, so I
checked the following ways,

1. Using of nodetag as an array of index to update the counter. But this
approach doesn't work for some cases like T_DropStmt where the tag
varies based on the removeType of the DropStmt. So I decide to drop
this approach.

2. Using of tag name for searching the fixed size array that is sorted with
all command tags that are possible in PostgreSQL. Using a binary search,
find out the location and update the counter. In this approach, first the
array
needs to be filed with TAG information in the sorted order.

3. Using of Hash table to store the counters information based on the TAG
name as key.

I choose the approach-3 as it can scale for any additional commands.

Any better alternatives with that directly it can point to the array index?

if we are going with ALTER XXX into single Alter counter is approach, then
I will change the code with a fixed array approach.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Robert Haas]

On Thu, Sep 29, 2016 at 1:45 AM, Haribabu Kommi
 wrote:
> Currently, The SQL stats is a fixed size counter to track the all the ALTER
> cases as single counter. So while sending the stats from the backend to
> stats collector at the end of the transaction, the cost is same, because of
> it's fixed size. This approach adds overhead to send and read the stats
> is minimal.
>
> With the following approach, I feel it is possible to support the counter at
> command tag level.
>
> Add a Global and local Hash to keep track of the counters by using the
> command tag as the key, this hash table increases dynamically whenever
> a new type of SQL command gets executed. The Local Hash data is passed
> to stats collector whenever the transaction gets committed.
>
> The problem I am thinking is that, Sending data from Hash and populating
> the Hash from stats file for all the command tags adds some overhead.

Yeah, I'm not very excited about that overhead.  This seems useful as
far as it goes, but I don't really want to incur measurable overhead
when it's in use.  Having a hash table rather than a fixed array of
slots means that you have to pass this through the stats collector
rather than updating shared memory directly, which is fairly heavy
weight.  If each backend could have its own copy of the slot array and
just update that, and readers added up the values across the whole
array, this could be done without any locking at all, and it would
generally be much lighter-weight than this approach.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[vinayak]

On 2016/10/17 10:22, Haribabu Kommi wrote:



On Fri, Oct 14, 2016 at 7:48 PM, vinayak 
> wrote:



On 2016/10/12 12:21, Haribabu Kommi wrote:

I tried changing the pg_stat_sql into row mode and ran the
regress suite to
add different type of SQL commands to the view and ran the
pgbench test
on my laptop to find out any performance impact with this patch.

HEAD  PATCH
pgbench - select  828  816

Here I attached the pg_stat_sql patch to keep track of all SQL
commands
based on the commandTag and their counts. I attached the result
of this
view that is run on the database after "make installcheck" just
for reference.

Some comments:
I think we can use pgstat_* instead of pgstat* for code consistency.

+static HTAB *pgStatBackendSql = NULL;
How about *pgstat_backend_sql

+HTAB   *pgStatSql = NULL;
How about *pgstat_sql


Changed.

+static void pgstat_recv_sqlstmt(PgStat_MsgSqlstmt * msg, int len);
How about PgStat_MsgSqlstmt *msg instead of PgStat_MsgSqlstmt * msg


Added the typdef into typdef.list file so this problem never occurs.

+typedef struct PgStatSqlstmtEntry
How about PgStat_SqlstmtEntry

+typedef struct PgStatMsgSqlstmt
How about PgStat_MsgSqlstmt



 changed.

Updated patch is attached.

The updated patch gives following error.
Error:
../../../../src/include/pgstat.h:557: error: ‘PgStatSqlstmtEntry’ 
undeclared here (not in a function)


-((PGSTAT_MSG_PAYLOAD - sizeof(int)) / sizeof(PgStatSqlstmtEntry))
+((PGSTAT_MSG_PAYLOAD - sizeof(int)) / sizeof(PgStat_SqlstmtEntry))
The attached patch fixed the error.

Regards,
Vinayak Pokale


pg_stat_sql_row_mode_2.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Fri, Oct 14, 2016 at 7:48 PM, vinayak 
wrote:

>
> On 2016/10/12 12:21, Haribabu Kommi wrote:
>
> I tried changing the pg_stat_sql into row mode and ran the regress suite
> to
> add different type of SQL commands to the view and ran the pgbench test
> on my laptop to find out any performance impact with this patch.
>
> HEAD  PATCH
> pgbench - select  828  816
>
> Here I attached the pg_stat_sql patch to keep track of all SQL commands
> based on the commandTag and their counts. I attached the result of this
> view that is run on the database after "make installcheck" just for
> reference.
>
> Some comments:
> I think we can use pgstat_* instead of pgstat* for code consistency.
>
> +static HTAB *pgStatBackendSql = NULL;
> How about *pgstat_backend_sql
>
> +HTAB   *pgStatSql = NULL;
> How about *pgstat_sql
>

Changed.


> +static void pgstat_recv_sqlstmt(PgStat_MsgSqlstmt * msg, int len);
> How about PgStat_MsgSqlstmt *msg instead of PgStat_MsgSqlstmt * msg
>

Added the typdef into typdef.list file so this problem never occurs.


> +typedef struct PgStatSqlstmtEntry
> How about PgStat_SqlstmtEntry
>
> +typedef struct PgStatMsgSqlstmt
> How about PgStat_MsgSqlstmt
>


 changed.


> I have observed below behavior.
> I have SET track_sql to ON and then execute the SELECT command and it
> return 0 rows.
> It start counting from the second command.
> postgres=# SET track_sql TO ON;
> SET
> postgres=# SELECT * FROM pg_stat_sql where tag='SELECT';
>  tag | count
> -+---
> (0 rows)
>
> postgres=# SELECT * FROM pg_stat_sql where tag='SELECT';
>   tag   | count
> +---
>  SELECT | 1
> (1 row)
> Is this a correct behavior?
>


Yes. This is because the stats of the SQL statements that are collected are
sent to
the stats collector at the end of the SQL statement execution. The current
SQL statement
is not counted in the result.

Updated patch is attached.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_sql_row_mode_2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Wed, Oct 12, 2016 at 4:06 PM, vinayak 
wrote:
>
>
> Thank you for the patch.
>
> Test: Commands with uppercase and lowercase
> 
> If the tag='select' then it returns the 0 rows but count is actually
> increment by 1.
>
> tag='select' vs tag='SELECT'
>
> postgres=# SET track_sql TO ON;
> SET
> postgres=# SELECT * FROM pg_stat_sql where tag='SELECT';
>   tag   | count
> +---
>  SELECT |12
> (1 row)
>
> postgres=# SELECT * FROM pg_stat_sql where tag=*'SELECT'*;
>   tag   | count
> +---
> * SELECT |13*
> (1 row)
>
> postgres=# SELECT * FROM pg_stat_sql where tag=*'select'*;
>  tag | count
> -+---
> *(0 rows)*
>
> postgres=# SELECT * FROM pg_stat_sql where tag=*'SELECT'*;
>   tag   | count
> +---
> * SELECT |15*
> (1 row)
>
> I think all command works same as above.
>

Thanks for checking the patch.

Yes, that's correct. Currently the CAPS letters are used as tag names
that are getting displayed whenever any SQL command is executed.
So I used the same names as entries to store the details of stats of
SQL statements.

If anyone feels the other way is better, I am fine for it.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[vinayak]

On 2016/10/12 12:21, Haribabu Kommi wrote:



On Thu, Sep 29, 2016 at 3:45 PM, Haribabu Kommi 
> wrote:




On Thu, Sep 22, 2016 at 3:05 AM, Alvaro Herrera
> wrote:

Peter Eisentraut wrote:

> How about having the tag not be a column name but a row
entry.  So you'd
> do something like
>
> SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';
>
> That way, we don't have to keep updating (and re-debating)
this when new
> command types or subtypes are added.  And queries written
for future
> versions will not fail when run against old servers.

Yeah, good idea.


Yes, Having it as a row entry is good.

Let's also discuss the interface from the stats collector. 
Currently we

have some 20 new SQL functions, all alike, each loading the
whole data
and returning a single counter, and then the view invokes each
function
separately.  That doesn't seem great to me.  How about having
a single C
function that returns the whole thing as a SRF instead, and
the view is
just a single function invocation -- something like pg_lock_status
filling pg_locks in one go.

Another consideration is that the present patch lumps together
all ALTER
cases in a single counter.  This isn't great, but at the same
time we
don't want to bloat the stat files by having hundreds of
counters per
database, do we?


Currently, The SQL stats is a fixed size counter to track the all
the ALTER
cases as single counter. So while sending the stats from the
backend to
stats collector at the end of the transaction, the cost is same,
because of
it's fixed size. This approach adds overhead to send and read the
stats
is minimal.

With the following approach, I feel it is possible to support the
counter at
command tag level.

Add a Global and local Hash to keep track of the counters by using the
command tag as the key, this hash table increases dynamically whenever
a new type of SQL command gets executed. The Local Hash data is passed
to stats collector whenever the transaction gets committed.

The problem I am thinking is that, Sending data from Hash and
populating
the Hash from stats file for all the command tags adds some overhead.



I tried changing the pg_stat_sql into row mode and ran the regress 
suite to

add different type of SQL commands to the view and ran the pgbench test
on my laptop to find out any performance impact with this patch.

HEAD  PATCH
pgbench - select  828  816

Here I attached the pg_stat_sql patch to keep track of all SQL commands
based on the commandTag and their counts. I attached the result of this
view that is run on the database after "make installcheck" just for 
reference.

Some comments:
I think we can use pgstat_* instead of pgstat* for code consistency.

+static HTAB *pgStatBackendSql = NULL;
How about *pgstat_backend_sql

+HTAB   *pgStatSql = NULL;
How about *pgstat_sql

+static void pgstat_recv_sqlstmt(PgStat_MsgSqlstmt * msg, int len);
How about PgStat_MsgSqlstmt *msg instead of PgStat_MsgSqlstmt * msg

+typedef struct PgStatSqlstmtEntry
How about PgStat_SqlstmtEntry

+typedef struct PgStatMsgSqlstmt
How about PgStat_MsgSqlstmt

I have observed below behavior.
I have SET track_sql to ON and then execute the SELECT command and it 
return 0 rows.

It start counting from the second command.
postgres=# SET track_sql TO ON;
SET
postgres=# SELECT * FROM pg_stat_sql where tag='SELECT';
 tag | count
-+---
(0 rows)

postgres=# SELECT * FROM pg_stat_sql where tag='SELECT';
  tag   | count
+---
 SELECT | 1
(1 row)
Is this a correct behavior?

Regards,
Vinayak Pokale
NTT Open Source Software Center

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[vinayak]

On 2016/10/12 12:21, Haribabu Kommi wrote:



On Thu, Sep 29, 2016 at 3:45 PM, Haribabu Kommi 
> wrote:




On Thu, Sep 22, 2016 at 3:05 AM, Alvaro Herrera
> wrote:

Peter Eisentraut wrote:

> How about having the tag not be a column name but a row
entry.  So you'd
> do something like
>
> SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';
>
> That way, we don't have to keep updating (and re-debating)
this when new
> command types or subtypes are added.  And queries written
for future
> versions will not fail when run against old servers.

Yeah, good idea.


Yes, Having it as a row entry is good.

Let's also discuss the interface from the stats collector. 
Currently we

have some 20 new SQL functions, all alike, each loading the
whole data
and returning a single counter, and then the view invokes each
function
separately.  That doesn't seem great to me.  How about having
a single C
function that returns the whole thing as a SRF instead, and
the view is
just a single function invocation -- something like pg_lock_status
filling pg_locks in one go.

Another consideration is that the present patch lumps together
all ALTER
cases in a single counter.  This isn't great, but at the same
time we
don't want to bloat the stat files by having hundreds of
counters per
database, do we?


Currently, The SQL stats is a fixed size counter to track the all
the ALTER
cases as single counter. So while sending the stats from the
backend to
stats collector at the end of the transaction, the cost is same,
because of
it's fixed size. This approach adds overhead to send and read the
stats
is minimal.

With the following approach, I feel it is possible to support the
counter at
command tag level.

Add a Global and local Hash to keep track of the counters by using the
command tag as the key, this hash table increases dynamically whenever
a new type of SQL command gets executed. The Local Hash data is passed
to stats collector whenever the transaction gets committed.

The problem I am thinking is that, Sending data from Hash and
populating
the Hash from stats file for all the command tags adds some overhead.



I tried changing the pg_stat_sql into row mode and ran the regress 
suite to

add different type of SQL commands to the view and ran the pgbench test
on my laptop to find out any performance impact with this patch.

HEAD  PATCH
pgbench - select  828  816

Here I attached the pg_stat_sql patch to keep track of all SQL commands
based on the commandTag and their counts. I attached the result of this
view that is run on the database after "make installcheck" just for 
reference.



Thank you for the patch.

Test: Commands with uppercase and lowercase

If the tag='select' then it returns the 0 rows but count is actually 
increment by 1.


tag='select' vs tag='SELECT'

postgres=# SET track_sql TO ON;
SET
postgres=# SELECT * FROM pg_stat_sql where tag='SELECT';
  tag   | count
+---
 SELECT |12
(1 row)

postgres=# SELECT * FROM pg_stat_sql where tag=*'SELECT'*;
  tag   | count
+---
* SELECT |13*
(1 row)

postgres=# SELECT * FROM pg_stat_sql where tag=*'select'*;
 tag | count
-+---
*(0 rows)*

postgres=# SELECT * FROM pg_stat_sql where tag=*'SELECT'*;
  tag   | count
+---
* SELECT |15*
(1 row)

I think all command works same as above.

Regards,
Vinayak Pokale
NTT Open Source Software Center

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Thu, Sep 29, 2016 at 3:45 PM, Haribabu Kommi 
wrote:

>
>
> On Thu, Sep 22, 2016 at 3:05 AM, Alvaro Herrera 
> wrote:
>
>> Peter Eisentraut wrote:
>>
>> > How about having the tag not be a column name but a row entry.  So you'd
>> > do something like
>> >
>> > SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';
>> >
>> > That way, we don't have to keep updating (and re-debating) this when new
>> > command types or subtypes are added.  And queries written for future
>> > versions will not fail when run against old servers.
>>
>> Yeah, good idea.
>>
>
> Yes, Having it as a row entry is good.
>
>
>
>> Let's also discuss the interface from the stats collector.  Currently we
>> have some 20 new SQL functions, all alike, each loading the whole data
>> and returning a single counter, and then the view invokes each function
>> separately.  That doesn't seem great to me.  How about having a single C
>> function that returns the whole thing as a SRF instead, and the view is
>> just a single function invocation -- something like pg_lock_status
>> filling pg_locks in one go.
>>
>> Another consideration is that the present patch lumps together all ALTER
>> cases in a single counter.  This isn't great, but at the same time we
>> don't want to bloat the stat files by having hundreds of counters per
>> database, do we?
>
>
> Currently, The SQL stats is a fixed size counter to track the all the
> ALTER
> cases as single counter. So while sending the stats from the backend to
> stats collector at the end of the transaction, the cost is same, because of
> it's fixed size. This approach adds overhead to send and read the stats
> is minimal.
>
> With the following approach, I feel it is possible to support the counter
> at
> command tag level.
>
> Add a Global and local Hash to keep track of the counters by using the
> command tag as the key, this hash table increases dynamically whenever
> a new type of SQL command gets executed. The Local Hash data is passed
> to stats collector whenever the transaction gets committed.
>
> The problem I am thinking is that, Sending data from Hash and populating
> the Hash from stats file for all the command tags adds some overhead.
>


I tried changing the pg_stat_sql into row mode and ran the regress suite to
add different type of SQL commands to the view and ran the pgbench test
on my laptop to find out any performance impact with this patch.

HEAD  PATCH
pgbench - select  828  816

Here I attached the pg_stat_sql patch to keep track of all SQL commands
based on the commandTag and their counts. I attached the result of this
view that is run on the database after "make installcheck" just for
reference.

Regards,
Hari Babu
Fujitsu Australia
   tag| count 
--+---
 ALTER TABLE  |   505
 DROP COLLATION   | 2
 ALTER POLICY |13
 CREATE TABLESPACE| 2
 DROP POLICY  |12
 CREATE TYPE  |78
 CREATE MATERIALIZED VIEW |24
 ALTER INDEX  | 8
 CREATE TEXT SEARCH PARSER| 5
 SELECT   |  8509
 REINDEX  | 7
 SET  |   638
 SHOW |63
 CREATE AGGREGATE |70
 DROP FUNCTION|   180
 DROP OPERATOR|12
 DROP USER MAPPING|23
 COMMENT  |55
 GRANT ROLE   | 9
 ALTER FOREIGN TABLE  |54
 CREATE DOMAIN|59
 DROP TABLE   |   481
 DROP SERVER  | 9
 DROP DOMAIN  |38
 INSERT   |  2663
 DROP INDEX   |56
 ALTER SERVER |17
 CREATE VIEW  |   260
 CREATE TABLE |  1023
 CREATE TEXT SEARCH CONFIGURATION |11
 ROLLBACK |   166
 ALTER DATABASE   | 5
 DISCARD ALL  | 1
 CLOSE CURSOR ALL | 2
 DROP EVENT TRIGGER   |11
 SAVEPOINT|62
 ALTER OPERATOR CLASS | 9
 DROP RULE|26
 DROP LANGUAGE| 3
 CREATE OPERATOR FAMILY   |20
 ALTER USER MAPPING   |12
 DROP SCHEMA  |29
 LISTEN   | 2
 DROP TEXT SEARCH PARSER  | 2
 CREATE TEXT SEARCH TEMPLATE  | 5
 DEALLOCATE   | 7
 CREATE FUNCTION  |   512
 REVOKE   |68
 CREATE TABLE AS  |52
 DROP MATERIALIZED VIEW   | 4
 

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Thu, Sep 22, 2016 at 3:05 AM, Alvaro Herrera 
wrote:

> Peter Eisentraut wrote:
>
> > How about having the tag not be a column name but a row entry.  So you'd
> > do something like
> >
> > SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';
> >
> > That way, we don't have to keep updating (and re-debating) this when new
> > command types or subtypes are added.  And queries written for future
> > versions will not fail when run against old servers.
>
> Yeah, good idea.
>

Yes, Having it as a row entry is good.



> Let's also discuss the interface from the stats collector.  Currently we
> have some 20 new SQL functions, all alike, each loading the whole data
> and returning a single counter, and then the view invokes each function
> separately.  That doesn't seem great to me.  How about having a single C
> function that returns the whole thing as a SRF instead, and the view is
> just a single function invocation -- something like pg_lock_status
> filling pg_locks in one go.
>
> Another consideration is that the present patch lumps together all ALTER
> cases in a single counter.  This isn't great, but at the same time we
> don't want to bloat the stat files by having hundreds of counters per
> database, do we?


Currently, The SQL stats is a fixed size counter to track the all the ALTER
cases as single counter. So while sending the stats from the backend to
stats collector at the end of the transaction, the cost is same, because of
it's fixed size. This approach adds overhead to send and read the stats
is minimal.

With the following approach, I feel it is possible to support the counter at
command tag level.

Add a Global and local Hash to keep track of the counters by using the
command tag as the key, this hash table increases dynamically whenever
a new type of SQL command gets executed. The Local Hash data is passed
to stats collector whenever the transaction gets committed.

The problem I am thinking is that, Sending data from Hash and populating
the Hash from stats file for all the command tags adds some overhead.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[David Fetter]

On Wed, Sep 21, 2016 at 02:05:24PM -0300, Alvaro Herrera wrote:
> Another consideration is that the present patch lumps together all
> ALTER cases in a single counter.  This isn't great, but at the same
> time we don't want to bloat the stat files by having hundreds of
> counters per database, do we?

I count 37 documented versions of ALTER as of git master.  Is there
some multiplier I'm missing?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Alvaro Herrera]

Peter Eisentraut wrote:

> How about having the tag not be a column name but a row entry.  So you'd
> do something like
> 
> SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';
> 
> That way, we don't have to keep updating (and re-debating) this when new
> command types or subtypes are added.  And queries written for future
> versions will not fail when run against old servers.

Yeah, good idea.

Let's also discuss the interface from the stats collector.  Currently we
have some 20 new SQL functions, all alike, each loading the whole data
and returning a single counter, and then the view invokes each function
separately.  That doesn't seem great to me.  How about having a single C
function that returns the whole thing as a SRF instead, and the view is
just a single function invocation -- something like pg_lock_status
filling pg_locks in one go.

Another consideration is that the present patch lumps together all ALTER
cases in a single counter.  This isn't great, but at the same time we
don't want to bloat the stat files by having hundreds of counters per
database, do we?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[David Fetter]

On Wed, Sep 21, 2016 at 11:25:14AM -0400, Peter Eisentraut wrote:
> On 9/14/16 4:01 PM, Robert Haas wrote:
> > I think it is not a good idea to make the command names used here the
> > plural forms of the command tags.  Instead of "inserts", "updates",
> > "imports", etc. just use "INSERT", "UPDATE", "IMPORT".  That's simpler
> > and less error prone - e.g. you won't end up with things like
> > "refreshs", which is not a word.
> 
> How about having the tag not be a column name but a row entry.  So you'd
> do something like
> 
> SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';

+1 for this.  It's MUCH easier to deal with changes in row counts than
changes in row type.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Peter Eisentraut]

On 9/14/16 4:01 PM, Robert Haas wrote:
> I think it is not a good idea to make the command names used here the
> plural forms of the command tags.  Instead of "inserts", "updates",
> "imports", etc. just use "INSERT", "UPDATE", "IMPORT".  That's simpler
> and less error prone - e.g. you won't end up with things like
> "refreshs", which is not a word.

How about having the tag not be a column name but a row entry.  So you'd
do something like

SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';

That way, we don't have to keep updating (and re-debating) this when new
command types or subtypes are added.  And queries written for future
versions will not fail when run against old servers.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Thu, Sep 15, 2016 at 6:01 AM, Robert Haas  wrote:

> On Fri, Sep 2, 2016 at 2:33 AM, Haribabu Kommi 
> wrote:
> > On Wed, Aug 31, 2016 at 3:19 AM, Alvaro Herrera <
> alvhe...@2ndquadrant.com>
> > wrote:
> >> Haribabu Kommi wrote:
> >>
> >>> Apart from the above, here are the following list of command tags that
> >>> are generated in the code, I took only the first word of the command
> tag
> >>> just to see how many categories present. The number indicates the
> >>> subset of operations or number of types it is used. Like create table,
> >>> create function and etc.
> >>
> >> Sounds about right.  I suppose all those cases that you aggregated here
> >> would expand to full tags in the actual code.  I furthermore suppose
> >> that some of these could be ignored, such as the transaction ones and
> >> things like load, lock, move, fetch, discard, deallocate (maybe lump
> >> them all together under "other", or some other rough categorization, as
> >> Tom suggests).
> >
> > Following is the pg_stat_sql view with the SQL categories that I
> considered
> > that are important. Rest of the them will be shown under others category.
> >
> > postgres=# \d pg_stat_sql
> >  View "pg_catalog.pg_stat_sql"
> >  Column  |   Type   | Modifiers
> > -+--+---
> >  inserts | bigint   |
> >  deletes | bigint   |
> >  updates | bigint   |
> >  selects | bigint   |
> >  declare_cursors | bigint   |
> >  closes  | bigint   |
> >  creates | bigint   |
> >  drops   | bigint   |
> >  alters  | bigint   |
> >  imports | bigint   |
> >  truncates   | bigint   |
> >  copies  | bigint   |
> >  grants  | bigint   |
> >  revokes | bigint   |
> >  clusters| bigint   |
> >  vacuums | bigint   |
> >  analyzes| bigint   |
> >  refreshs| bigint   |
> >  locks   | bigint   |
> >  checkpoints | bigint   |
> >  reindexes   | bigint   |
> >  deallocates | bigint   |
> >  others  | bigint   |
> >  stats_reset | timestamp with time zone |
> >
> >
> > If any additions/deletions, I can accommodate them.
>
> I think it is not a good idea to make the command names used here the
> plural forms of the command tags.  Instead of "inserts", "updates",
> "imports", etc. just use "INSERT", "UPDATE", "IMPORT".  That's simpler
> and less error prone - e.g. you won't end up with things like
> "refreshs", which is not a word.


Thanks for your suggestion. I also thought of changing the name while
writing
"refreshs" as a column name of the view originally. A small restriction
with the
change of names to command names is that, user needs to execute the query
as follows.

select pg_stat_sql.select from pg_stat_sql;

Updated patch is attached with the corrections. Apart from name change,
added documentation and tests.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_sql_2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Robert Haas]

On Fri, Sep 2, 2016 at 2:33 AM, Haribabu Kommi  wrote:
> On Wed, Aug 31, 2016 at 3:19 AM, Alvaro Herrera 
> wrote:
>> Haribabu Kommi wrote:
>>
>>> Apart from the above, here are the following list of command tags that
>>> are generated in the code, I took only the first word of the command tag
>>> just to see how many categories present. The number indicates the
>>> subset of operations or number of types it is used. Like create table,
>>> create function and etc.
>>
>> Sounds about right.  I suppose all those cases that you aggregated here
>> would expand to full tags in the actual code.  I furthermore suppose
>> that some of these could be ignored, such as the transaction ones and
>> things like load, lock, move, fetch, discard, deallocate (maybe lump
>> them all together under "other", or some other rough categorization, as
>> Tom suggests).
>
> Following is the pg_stat_sql view with the SQL categories that I considered
> that are important. Rest of the them will be shown under others category.
>
> postgres=# \d pg_stat_sql
>  View "pg_catalog.pg_stat_sql"
>  Column  |   Type   | Modifiers
> -+--+---
>  inserts | bigint   |
>  deletes | bigint   |
>  updates | bigint   |
>  selects | bigint   |
>  declare_cursors | bigint   |
>  closes  | bigint   |
>  creates | bigint   |
>  drops   | bigint   |
>  alters  | bigint   |
>  imports | bigint   |
>  truncates   | bigint   |
>  copies  | bigint   |
>  grants  | bigint   |
>  revokes | bigint   |
>  clusters| bigint   |
>  vacuums | bigint   |
>  analyzes| bigint   |
>  refreshs| bigint   |
>  locks   | bigint   |
>  checkpoints | bigint   |
>  reindexes   | bigint   |
>  deallocates | bigint   |
>  others  | bigint   |
>  stats_reset | timestamp with time zone |
>
>
> If any additions/deletions, I can accommodate them.

I think it is not a good idea to make the command names used here the
plural forms of the command tags.  Instead of "inserts", "updates",
"imports", etc. just use "INSERT", "UPDATE", "IMPORT".  That's simpler
and less error prone - e.g. you won't end up with things like
"refreshs", which is not a word.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Wed, Aug 31, 2016 at 3:19 AM, Alvaro Herrera 
wrote:
> Haribabu Kommi wrote:
>
>> Apart from the above, here are the following list of command tags that
>> are generated in the code, I took only the first word of the command tag
>> just to see how many categories present. The number indicates the
>> subset of operations or number of types it is used. Like create table,
>> create function and etc.
>
> Sounds about right.  I suppose all those cases that you aggregated here
> would expand to full tags in the actual code.  I furthermore suppose
> that some of these could be ignored, such as the transaction ones and
> things like load, lock, move, fetch, discard, deallocate (maybe lump
> them all together under "other", or some other rough categorization, as
> Tom suggests).

Following is the pg_stat_sql view with the SQL categories that I considered
that are important. Rest of the them will be shown under others category.

postgres=# \d pg_stat_sql
 View "pg_catalog.pg_stat_sql"
 Column  |   Type   | Modifiers
-+--+---
 inserts | bigint   |
 deletes | bigint   |
 updates | bigint   |
 selects | bigint   |
 declare_cursors | bigint   |
 closes  | bigint   |
 creates | bigint   |
 drops   | bigint   |
 alters  | bigint   |
 imports | bigint   |
 truncates   | bigint   |
 copies  | bigint   |
 grants  | bigint   |
 revokes | bigint   |
 clusters| bigint   |
 vacuums | bigint   |
 analyzes| bigint   |
 refreshs| bigint   |
 locks   | bigint   |
 checkpoints | bigint   |
 reindexes   | bigint   |
 deallocates | bigint   |
 others  | bigint   |
 stats_reset | timestamp with time zone |


If any additions/deletions, I can accommodate them.

The stats data gets updated in exec_simple_query and exec_execute_message
functions and the collected stats will be sent to stats collector similar
like function usage stats in pgstat_report_stat function.

These SQL statistics data is stored in the stats file similar like global
statistics. The STAT file format is updated to accommodate the new stats.

A new GUC "track_sql" is added to track the SQL statement
statistics, by default this is off. Only superuser can change this
parameter.

Attached a patch for the same.

>  Also, for many of these commands it's probably relevant
> whether they are acting on a temporary object or not; we should either
> count these separately, or not count the temp ones at all.

Currently the SQL stats are not checking any object level that is a temp
one or not? The temp objects are specific to a backend only. But what
I feel, any way that is an SQL query that was executed on a temp object,
so we need to count that operation.

I feel this SQL stats should not worry about the object type. May be I am
wrong.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_sql_1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Alvaro Herrera]

Haribabu Kommi wrote:

> Apart from the above, here are the following list of command tags that
> are generated in the code, I took only the first word of the command tag
> just to see how many categories present. The number indicates the
> subset of operations or number of types it is used. Like create table,
> create function and etc.

Sounds about right.  I suppose all those cases that you aggregated here
would expand to full tags in the actual code.  I furthermore suppose
that some of these could be ignored, such as the transaction ones and
things like load, lock, move, fetch, discard, deallocate (maybe lump
them all together under "other", or some other rough categorization, as
Tom suggests).  Also, for many of these commands it's probably relevant
whether they are acting on a temporary object or not; we should either
count these separately, or not count the temp ones at all.

> insert
> delete
> update
> select (6)
> transaction (10)
> declare
> close (2)
> move
> fetch
> create (37)
> drop (36)
> Alter (56)
> import
> truncate
> comment
> security
> copy
> grant
> revoke
> notify
> listen
> unlisten
> load
> cluster
> vacuum
> analyze
> explain
> refresh
> set (2)
> reset
> show
> discard (4)
> reassign
> lock
> checkpoint
> reindex
> prepare
> execute
> deallocate

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Tue, Aug 23, 2016 at 3:59 AM, Andres Freund  wrote:
>> Haribabu's categorization
>> scheme seems to need some work, but the idea of categorizing
>> statements by type and counting executions per type seems very
>> reasonable.
>
> I'd consider instead using something like COALESCE(commandType,
> nodeTag(Query->utilityStmt)) as the categories. Not sure if I'd even
> pivot that.

CommandType means to use select, insert, update, delete and utility
as the categorization to display the counters? I think I am not getting
your point correctly.

Apart from the above, here are the following list of command tags that
are generated in the code, I took only the first word of the command tag
just to see how many categories present. The number indicates the
subset of operations or number of types it is used. Like create table,
create function and etc.

insert
delete
update
select (6)
transaction (10)
declare
close (2)
move
fetch
create (37)
drop (36)
Alter (56)
import
truncate
comment
security
copy
grant
revoke
notify
listen
unlisten
load
cluster
vacuum
analyze
explain
refresh
set (2)
reset
show
discard (4)
reassign
lock
checkpoint
reindex
prepare
execute
deallocate

we can try to pick something from the above list also for main
categories and rest will fall under
other.


Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[neha khatri]

On Wed, Aug 24, 2016 at 10:07 AM, Gavin Flower <
gavinflo...@archidevsys.co.nz> wrote:

> On 24/08/16 12:02, neha khatri wrote:
>
>> >Andres Freund > writes:
>> >> On 2016-08-22 13:54:43 -0400, Robert Haas wrote:
>> >> On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane  t...@sss.pgh.pa.us>> wrote:
>>  I'm inclined to suggest you forget this approach and propose a single
>>  counter for "SQL commands executed", which avoids all of the above
>>  definitional problems.  People who need more detail than that are
>>  probably best advised to look to contrib/pg_stat_statements, anyway.
>>
>> >>> I disagree.  I think SQL commands executed, lumping absolutely
>> >>> everything together, really isn't much use.
>>
>> >> I'm inclined to agree. I think that's a quite useful stat when looking
>> >> at an installation one previously didn't have a lot of interaction
>> with.
>>
>> >Well, let's at least have an "other" category so you can add up the
>> >counters and get a meaningful total.
>>
>> How would that meaningful total might help a user. What can a user might
>> analyse with the counter in 'other' category.
>>
>>
>>
>> The user could then judge if there were a significant number of examples
> not covered in the other categories - this may, or may not, be a problem;
> depending on the use case.
>
>
> Cheers,
> Gavin
>
> For the user to be able to judge that whether the number in the 'other'
category is a problem or not, the user is also required to know what all
might fall under the 'other' category. It may not be good to say that
_anything_ that is not part of the already defined category is part of
'other'. Probably, 'other' should also be a set of predefined operations.

Neha

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Gavin Flower]

On 24/08/16 12:02, neha khatri wrote:

>Andres Freund > writes:
>> On 2016-08-22 13:54:43 -0400, Robert Haas wrote:
>> On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane > wrote:

 I'm inclined to suggest you forget this approach and propose a single
 counter for "SQL commands executed", which avoids all of the above
 definitional problems.  People who need more detail than that are
 probably best advised to look to contrib/pg_stat_statements, anyway.

>>> I disagree.  I think SQL commands executed, lumping absolutely
>>> everything together, really isn't much use.

>> I'm inclined to agree. I think that's a quite useful stat when looking
>> at an installation one previously didn't have a lot of interaction 
with.


>Well, let's at least have an "other" category so you can add up the
>counters and get a meaningful total.

How would that meaningful total might help a user. What can a user 
might analyse with the counter in 'other' category.



Neha

The user could then judge if there were a significant number of examples 
not covered in the other categories - this may, or may not, be a 
problem; depending on the use case.



Cheers,
Gavin



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[neha khatri]

>Andres Freund  writes:
>> On 2016-08-22 13:54:43 -0400, Robert Haas wrote:
>> On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane  wrote:
 I'm inclined to suggest you forget this approach and propose a single
 counter for "SQL commands executed", which avoids all of the above
 definitional problems.  People who need more detail than that are
 probably best advised to look to contrib/pg_stat_statements, anyway.

>>> I disagree.  I think SQL commands executed, lumping absolutely
>>> everything together, really isn't much use.

>> I'm inclined to agree. I think that's a quite useful stat when looking
>> at an installation one previously didn't have a lot of interaction with.

>Well, let's at least have an "other" category so you can add up the
>counters and get a meaningful total.

How would that meaningful total might help a user. What can a user might
analyse with the counter in 'other' category.


Neha

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Gavin Flower]

On 23/08/16 08:27, Tom Lane wrote:

Andres Freund  writes:

On 2016-08-22 13:54:43 -0400, Robert Haas wrote:

On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane  wrote:

I'm inclined to suggest you forget this approach and propose a single
counter for "SQL commands executed", which avoids all of the above
definitional problems.  People who need more detail than that are
probably best advised to look to contrib/pg_stat_statements, anyway.

I disagree.  I think SQL commands executed, lumping absolutely
everything together, really isn't much use.

I'm inclined to agree. I think that's a quite useful stat when looking
at an installation one previously didn't have a lot of interaction with.

Well, let's at least have an "other" category so you can add up the
counters and get a meaningful total.

regards, tom lane


Initially I thought of something I thought was factious, but then 
realized it might actually be both practicable & useful...


How about 2 extra categories (if appropriate!!!):

1. Things that actually might be sort of as fitting in 2 or more of the
   existing categories, or there is an ambiguity as to which category
   is appropriate.

2. Things that don't fit into any existing category

This was inspired by a real use case, in a totally unrelated area - but 
where I attempted to ensure counts were in the most useful categories I 
was able to provide.  The user had listed categories, but I found that 
the data didn't always fit neatly into them as specified.



Cheers,

Gavin




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Tom Lane]

Andres Freund  writes:
> On 2016-08-22 13:54:43 -0400, Robert Haas wrote:
>> On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane  wrote:
>>> I'm inclined to suggest you forget this approach and propose a single
>>> counter for "SQL commands executed", which avoids all of the above
>>> definitional problems.  People who need more detail than that are
>>> probably best advised to look to contrib/pg_stat_statements, anyway.

>> I disagree.  I think SQL commands executed, lumping absolutely
>> everything together, really isn't much use.

> I'm inclined to agree. I think that's a quite useful stat when looking
> at an installation one previously didn't have a lot of interaction with.

Well, let's at least have an "other" category so you can add up the
counters and get a meaningful total.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Andres Freund]

On 2016-08-22 13:54:43 -0400, Robert Haas wrote:
> On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane  wrote:
> > I'm inclined to suggest you forget this approach and propose a single
> > counter for "SQL commands executed", which avoids all of the above
> > definitional problems.  People who need more detail than that are
> > probably best advised to look to contrib/pg_stat_statements, anyway.
> 
> I disagree.  I think SQL commands executed, lumping absolutely
> everything together, really isn't much use.

I'm inclined to agree. I think that's a quite useful stat when looking
at an installation one previously didn't have a lot of interaction with.


> Haribabu's categorization
> scheme seems to need some work, but the idea of categorizing
> statements by type and counting executions per type seems very
> reasonable.

I'd consider instead using something like COALESCE(commandType,
nodeTag(Query->utilityStmt)) as the categories. Not sure if I'd even
pivot that.

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Robert Haas]

On Sat, Aug 20, 2016 at 11:17 AM, Tom Lane  wrote:
> I'm inclined to suggest you forget this approach and propose a single
> counter for "SQL commands executed", which avoids all of the above
> definitional problems.  People who need more detail than that are
> probably best advised to look to contrib/pg_stat_statements, anyway.

I disagree.  I think SQL commands executed, lumping absolutely
everything together, really isn't much use.  Haribabu's categorization
scheme seems to need some work, but the idea of categorizing
statements by type and counting executions per type seems very
reasonable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Sun, Aug 21, 2016 at 1:17 AM, Tom Lane  wrote:
> Haribabu Kommi  writes:
>> This is a new statistics view that is used to provide the number of
>> SQL operations that are
>> happened on a particular interval of time. This view is useful for the
>> system to find out the
>> pattern of the operations that are happening in the instance during
>> particular interval of
>> time.
>
> 1. This set of counters seems remarkably random.  Why, for instance,
> count reindexes but not original index creations?

I thought of adding the columns to the view that are mostly used(excluding
all DDL commands), otherwise the view columns fill with all the commands
that are supported by PostgreSQL.

> 2. What will you do with cases such as SELECTs containing modifying CTEs?
> Or EXPLAIN ANALYZE?  Does CLUSTER count as a REINDEX?  Does REFRESH
> MATERIALIZED VIEW count as a SELECT?  (Or maybe it's an INSERT?)

Cluster commands will be calculated as clusters, explain needs some
logic to identify what SQL operation exactly. This is because of using
nodeTag data from the parseTree structure to identify what type of SQL
statement it is.

The counters will not updated for any SQL type that is not as part of the view.

> If you're going to be selective about what you count, you're forever
> going to be fielding complaints from users who are unhappy that you
> didn't count some statement type they care about, or feel that you
> misclassified some complex case.

Yes, I agree that users may complain regarding with limited columns that are not
sufficient for their use. But with the mostly used columns, this view
may be useful for
some of the users to find out the pattern of the SQL operations that
are happening
on the instance.

> I'm inclined to suggest you forget this approach and propose a single
> counter for "SQL commands executed", which avoids all of the above
> definitional problems.  People who need more detail than that are
> probably best advised to look to contrib/pg_stat_statements, anyway.

I will add a new column to the pg_stat_database, to track the number of SQL
operations that are happened on this particular database.

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Tom Lane]

Haribabu Kommi  writes:
> This is a new statistics view that is used to provide the number of
> SQL operations that are
> happened on a particular interval of time. This view is useful for the
> system to find out the
> pattern of the operations that are happening in the instance during
> particular interval of
> time.

> Following is the more or less columns and their details of the pg_stat_sql 
> view.

> postgres=# \d pg_stat_sql
>View "pg_catalog.pg_stat_sql"
>Column|   Type   | Modifiers
> -+--+---
>  selects | bigint   |
>  inserts | bigint   |
>  deletes | bigint   |
>  updates | bigint   |
>  declares| bigint   |
>  fetches | bigint   |
>  copies  | bigint   |
>  reindexes   | bigint   |
>  truncates   | bigint   |
>  stats_reset | timestamp with time zone |


1. This set of counters seems remarkably random.  Why, for instance,
count reindexes but not original index creations?

2. What will you do with cases such as SELECTs containing modifying CTEs?
Or EXPLAIN ANALYZE?  Does CLUSTER count as a REINDEX?  Does REFRESH
MATERIALIZED VIEW count as a SELECT?  (Or maybe it's an INSERT?)

If you're going to be selective about what you count, you're forever
going to be fielding complaints from users who are unhappy that you
didn't count some statement type they care about, or feel that you
misclassified some complex case.

I'm inclined to suggest you forget this approach and propose a single
counter for "SQL commands executed", which avoids all of the above
definitional problems.  People who need more detail than that are
probably best advised to look to contrib/pg_stat_statements, anyway.

Also, if you do that, there hardly seems a need for a whole new view.
You could just add a column to pg_stat_database.  The incremental
maintenance effort doesn't seem enough to justify its own GUC, either.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

This is a new statistics view that is used to provide the number of
SQL operations that are
happened on a particular interval of time. This view is useful for the
system to find out the
pattern of the operations that are happening in the instance during
particular interval of
time.

Following is the more or less columns and their details of the pg_stat_sql view.

postgres=# \d pg_stat_sql
   View "pg_catalog.pg_stat_sql"
   Column|   Type   | Modifiers
-+--+---
 selects | bigint   |
 inserts | bigint   |
 deletes | bigint   |
 updates | bigint   |
 declares| bigint   |
 fetches | bigint   |
 copies  | bigint   |
 reindexes   | bigint   |
 truncates   | bigint   |
 stats_reset | timestamp with time zone |

The SQL counters gets updated only for the external SQL queries, not for
the SQL queries that are generated internally. The counters gets updated
at exec_simple_query and exec_execute_message functions.

User can reset the SQL counter statistics using an exposed function.
The Stats collection can be turned on/off using a GUC also.

Any comments/objections in providing a patch for the same?

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers