Request improve pg_stat_statements module

Started by Nonamealmost 12 years ago3 messages

pgsql-kr@postgresql.kr

almost 12 years ago

I patched to add one column in pg_stat_statements module.
and sent to author but
I recived a reject mail because unknown user :(

so I am posting to this mailling.

I need a last time of query, because I want to analyse order by recent time.

this patch code below,
review please and
I wish to apply at next version.

--- diff begin
--- ../pg_stat_statements.orig/pg_stat_statements.c     2014-02-18 04:29:55.000000000 +0900
+++ pg_stat_statements.c        2014-02-28 15:34:38.000000000 +0900
@@ -59,6 +59,7 @@
 #include "storage/spin.h"
 #include "tcop/utility.h"
 #include "utils/builtins.h"
+#include "utils/timestamp.h"

 PG_MODULE_MAGIC;
@@ -116,6 +117,7 @@
        double          blk_read_time;  /* time spent reading, in msec */
        double          blk_write_time; /* time spent writing, in msec */
        double          usage;                  /* usage factor */
+       TimestampTz     last_executed_timestamp;         /* last executed timestamp of query */
 } Counters;

 /*
@@ -1043,6 +1045,8 @@
                e->counters.blk_read_time += INSTR_TIME_GET_MILLISEC(bufusage->blk_read_time);
                e->counters.blk_write_time += INSTR_TIME_GET_MILLISEC(bufusage->blk_write_time);
                e->counters.usage += USAGE_EXEC(total_time);
+               /* last executed timestamp */
+               e->counters.last_executed_timestamp = GetCurrentTimestamp();

SpinLockRelease(&e->mutex);
}
@@ -1069,7 +1073,8 @@
}

 #define PG_STAT_STATEMENTS_COLS_V1_0   14
-#define PG_STAT_STATEMENTS_COLS                        18
+#define PG_STAT_STATEMENTS_COLS_V1_1   18
+#define PG_STAT_STATEMENTS_COLS                19

 /*
  * Retrieve statement statistics.
@@ -1087,6 +1092,7 @@
        HASH_SEQ_STATUS hash_seq;
        pgssEntry  *entry;
        bool            sql_supports_v1_1_counters = true;
+       bool            sql_supports_v1_2_counters = true;

        if (!pgss || !pgss_hash)
                ereport(ERROR,
@@ -1107,8 +1113,12 @@
        /* Build a tuple descriptor for our result type */
        if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
                elog(ERROR, "return type must be a row type");
-       if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_0)
+       if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_0){
                sql_supports_v1_1_counters = false;
+               sql_supports_v1_2_counters = false;
+       }
+       if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_1)
+               sql_supports_v1_2_counters = false;

        per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
        oldcontext = MemoryContextSwitchTo(per_query_ctx);
@@ -1185,8 +1195,15 @@
                        values[i++] = Float8GetDatumFast(tmp.blk_read_time);
                        values[i++] = Float8GetDatumFast(tmp.blk_write_time);
                }
+               // last_executed_timestamp
+               if (sql_supports_v1_2_counters)
+                       values[i++] = TimestampTzGetDatum(tmp.last_executed_timestamp);
+

-               Assert(i == (sql_supports_v1_1_counters ?
+               if(sql_supports_v1_2_counters)
+                       Assert(i == PG_STAT_STATEMENTS_COLS);
+               else
+                       Assert(i == (sql_supports_v1_1_counters ?
                                         PG_STAT_STATEMENTS_COLS : PG_STAT_STATEMENTS_COLS_V1_0));

tuplestore_putvalues(tupstore, tupdesc, values, nulls);

-- end of diff

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

Michael Paquier

michael.paquier@gmail.com

almost 12 years ago

In reply to: Noname (#1)

Re: Request improve pg_stat_statements module

Thanks for your patch!

On Fri, Feb 28, 2014 at 4:18 PM, <pgsql-kr@postgresql.kr> wrote:

I patched to add one column in pg_stat_statements module.
and sent to author but
I need a last time of query, because I want to analyse order by recent time.

Hm... I am not sure that this brings much to pg_stat_statements, which
is interested to gather normalized information about the queries run
on the server. For example, things like calculating the average time
of a query by using total_time/calls or even the total time to guess
where is an application bottleneck is interesting on a busy system,
while the latest timestamp is not really an information that can be
used for query tuning. Getting the latest timestamp when a query has
run is particularly not interesting on OLTP-like applications where
many short transactions are running. It is more interesting to
classify query results by either calls, total_time or the combination
of both IMO.

this patch code below, review please and
I wish to apply at next version.

When submitting patches, there are a couple of things to know, they
are summarized here.
https://wiki.postgresql.org/wiki/Submitting_a_Patch

One of the things that would be interesting for future contributions
in your case is knowing the code convention used in Postgres code (see
comments below for more details):
http://www.postgresql.org/docs/devel/static/source.html

-       if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_0)
+       if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_0){
sql_supports_v1_1_counters = false;
+               sql_supports_v1_2_counters = false;
+       }

This is incorrect style, please create a new like for a bracket, like that:
if (cond)
{
blabla;
blabla2;
}

per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
oldcontext = MemoryContextSwitchTo(per_query_ctx);
@@ -1185,8 +1195,15 @@
values[i++] = Float8GetDatumFast(tmp.blk_read_time);
values[i++] = Float8GetDatumFast(tmp.blk_write_time);
}
+               // last_executed_timestamp

Please do not use double-slashes for comments, write them instead like that:
/* my comment is here */

Finally be sure to attach a patch to an email and avoid to simply
copy/paste the content of the patch in your email. This is more
user-friendly :) Context diffs are recommended as well.

Regards,
--
Michael

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

Robert Haas

robertmhaas@gmail.com

almost 12 years ago

In reply to: Michael Paquier (#2)

Re: Request improve pg_stat_statements module

On Fri, Feb 28, 2014 at 8:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Thanks for your patch!

On Fri, Feb 28, 2014 at 4:18 PM, <pgsql-kr@postgresql.kr> wrote:

I patched to add one column in pg_stat_statements module.
and sent to author but
I need a last time of query, because I want to analyse order by recent time.

Hm... I am not sure that this brings much to pg_stat_statements, which
is interested to gather normalized information about the queries run
on the server. For example, things like calculating the average time
of a query by using total_time/calls or even the total time to guess
where is an application bottleneck is interesting on a busy system,
while the latest timestamp is not really an information that can be
used for query tuning. Getting the latest timestamp when a query has
run is particularly not interesting on OLTP-like applications where
many short transactions are running. It is more interesting to
classify query results by either calls, total_time or the combination
of both IMO.

FWIW, I think it's a neat idea, but I think this proposal is probably
too late for 9.4.

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