Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

From: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
To: Julian Markwort <julian(dot)markwort(at)uni-muenster(dot)de>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)anarazel(dot)de>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, marius(dot)timmer(at)uni-muenster(dot)de, arne(dot)scheffer(at)uni-muenster(dot)de
Subject: Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)
Date: 2018-03-09 19:17:21
Message-ID: 20180309191720.GA20156@arthur.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Thu, Mar 08, 2018 at 02:58:34PM +0100, Julian Markwort wrote:
> > I'd love to hear more feedback, here are two ideas to polish this
> > patch:
> > a) Right now, good_plan and bad_plan collection can be activated and
> > deactivated with separate GUCs. I think it would be sensible to
> > collect
> > either both or none. (This would result in fewer convoluted
> > conditionals.)
> > b) Would you like to be able to tune the percentiles yourself, to
> > adjust for the point at which a new plan is stored?

I'd like to review the patch and leave a feedback. I tested it with
different indexes on same table and with same queries and it works fine.

First of all, GUC variables and functions. How about union
'pg_stat_statements.good_plan_enable' and
'pg_stat_statements.bad_plan_enable' into
'pg_stat_statements.track_plan' with GUC_LIST_INPUT flag? It may accept
comma separated values 'good' and 'bad'. It lets to add another tracking
type in future without adding new variable.

In same manner pg_stat_statements_good_plan_reset() and
pg_stat_statements_bad_plan_reset() functions can be combined in

pg_stat_statements_plan_reset(in queryid bigint, in good boolean, in bad

Further comments on the code.

+-- test to see if any plans have been recorded.
+ CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+ CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+ CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END,
+ CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END

I think here is a typo. Last case should be bad_plan_timestamp.

+ good_plan_str = palloc(1 * sizeof(char));
+ *good_plan_str = '\0';
+ bad_plan_str = palloc(1 * sizeof(char));
+ *bad_plan_str = '\0';

Here we can use empty string literals instead of pallocs. For example:

const char *good_plan_str;
const char *bad_plan_str;
good_plan_str = "";
bad_plan_str = "";

+ interquartile_dist = 2.0*(0.6745 * sqrt(e->counters.sum_var_time / e->counters.calls));

It is worth to check e->counters.calls for zero here. Because the entry
can be sticky.

Arthur Zakirov
Postgres Professional:
Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Verite 2018-03-09 19:20:18 Re: csv format for psql
Previous Message David Steele 2018-03-09 18:51:14 Re: PATCH: Configurable file mode mask