On Mon, Jun 29, 2020 at 1:14 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > > On 2020/06/29 18:56, Fujii Masao wrote: > > > > > > On 2020/06/29 18:53, Julien Rouhaud wrote: > >> On Mon, Jun 29, 2020 at 11:38 AM Fujii Masao > >> <masao.fu...@oss.nttdata.com> wrote: > >>> > >>>>> Your benchmark result seems to suggest that the cause of the problem is > >>>>> the contention of per-query spinlock in pgss_store(). Right? > >>>>> This lock contention is likely to happen when multiple sessions run > >>>>> the same queries. > >>>>> > >>>>> One idea to reduce that lock contention is to separate per-query > >>>>> spinlock > >>>>> into two; one is for planning, and the other is for execution. > >>>>> pgss_store() > >>>>> determines which lock to use based on the given "kind" argument. > >>>>> To make this idea work, also every pgss counters like shared_blks_hit > >>>>> need to be separated into two, i.e., for planning and execution. > >>>> > >>>> This can probably remove some overhead, but won't it eventually hit > >>>> the same issue when multiple connections try to plan the same query, > >>>> given the number of different queries and very low execution runtime? > >>> > >>> Yes. But maybe we can expect that the idea would improve > >>> the performance to the near same level as v12? > >> > >> A POC patch should be easy to do and see how much it solves this > >> problem. However I'm not able to reproduce the issue, and IMHO unless > >> we specifically want to be able to distinguish planner-time counters > >> from execution-time counters, I'd prefer to disable track_planning by > >> default than going this way, so that users with a sane usage won't > >> have to suffer from a memory increase. > > > > Agreed. +1 to change that default to off. > > Attached patch does this.
Patch looks good to me. > I also add the following into the description about each *_plan_time column > in the docs. IMO this is helpful for users when they see that those columns > report zero by default and try to understand why. > > (if <varname>pg_stat_statements.track_planning</varname> is enabled, > otherwise zero) +1 Do you intend to wait for other input before pushing? FWIW I'm still not convinced that the exposed problem is representative of any realistic workload. I of course entirely agree with the other documentation changes.
