Re: Request improve pg_stat_statements module - Mailing list pgsql-hackers

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