Thread
-
Request improve pg_stat_statements module
Ioseph Kim <pgsql-kr@postgresql.kr> — 2014-02-28T07:18:48Z
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 -
Re: Request improve pg_stat_statements module
Michael Paquier <michael.paquier@gmail.com> — 2014-02-28T13:01:22Z
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 -
Re: Request improve pg_stat_statements module
Robert Haas <robertmhaas@gmail.com> — 2014-03-03T15:36:35Z
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