On Fri, Jan 19, 2018 at 12:14 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Sat, Jan 13, 2018 at 2:16 PM, Tom Lane <tgl@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