On Sat, Jan 13, 2018 at 2:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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,
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
