Re: [HACKERS] Planning counters in pg_stat_statements

I wrote:
> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>> I checked the latest patch and it is working fine and I don't have any
>> further comments. Marked the patch as "ready for committer".

> I started to look at this patch,

... looking further, I'm really seriously unhappy about this bit:

@@ -943,9 +1006,16 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
...
+
+        /*
+         * Clear planning_time, so that we only count it once for each
+         * replanning of a prepared statement.
+         */
+        queryDesc->plannedstmt->planning_time = 0;
     }

What we have here is pgss_ExecutorEnd stomping on the plan cache.
I do *not* find that acceptable.  At the very least, it ruins the
theory that this field is a shared resource.

What we could/should do instead, I think, is have pgss_planner_hook
make its own pgss_store() call to log the planning time results
(which would mean we don't need the added PlannedStmt field at all).
That would increase the overhead of this feature, which might mean
that it'd be worth having a pg_stat_statements GUC to enable it.
But it'd be a whole lot cleaner.

            regards, tom lane