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
