On Sat, Jan 13, 2018 at 2:16 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > I wrote: >> Haribabu Kommi <kommi.harib...@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,
Thanks! > ... 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. The idea was that planning_time is work space for extensions to do whatever they like with, but objection noted. > 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. Ok. It seems a bit strange to have to go through the locking twice, but I don't have a better idea. First attempt seems to be working. I'll post an updated patch in a couple of days when I'm back from travelling and have a tidier version with a new GUC and have thought about the nested_level question. -- Thomas Munro http://www.enterprisedb.com
