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

Greetings,

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

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

Further comments on the code.

+-- test to see if any plans have been recorded.
+SELECT
+ 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: http://www.postgrespro.com
Russian Postgres Company