On Fri, Jan 19, 2018 at 12:14 AM, Thomas Munro <thomas.mu...@enterprisedb.com> wrote: > On Sat, Jan 13, 2018 at 2:16 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: >> 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.
One problem is that pgss_planner_hook doesn't have the source query text. That problem could be solved by adding a query_string argument to the planner hook function type and also planner(), standard_planner(), pg_plan_query(), pg_plan_queries(). I don't know if that change would be acceptable or if there is a better way that doesn't change extern functions that will annoy extension owners. When I tried that it basically worked except for a problem with "PREPARE ... <actual query>" (what we want to show) vs "<actual query>" (what my experimental patch now shows -- oops). Something I wondered about but didn't try: we could skip the above problem AND the extra pgss_store() by instead pushing (query ID, planning time) into a backend-private buffer and then later pushing it into shmem when we eventually call pgss_store() for the execution counters. If you think of that as using global variables to pass state between functions just because we didn't structure our program right it sounds terrible, but if you think of it as a dtrace-style per-thread tracing event buffer that improves performance by batching up collected data, it sounds great :-D Unfortunately I'm not going to have bandwidth to figure this out during this commitfest due to other priorities so I'm going to call this "returned with feedback". -- Thomas Munro http://www.enterprisedb.com