Re: track_planning causing performance regression

On Mon, Jun 29, 2020 at 1:14 PM Fujii Masao <masao.fujii@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.fujii@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.