MIME-Version: 1.0
References: <2895b53b033c47ccb22972b589050dd9@EX13D05UWC001.ant.amazon.com>
 <CAOBaU_ZNAjSQEdWK9ERsv6zqhdRqB_C0dVv97BrKETV4Rv7gig@mail.gmail.com>
 <d4a15deb-8a85-b78c-b447-bc5df7c4a926@oss.nttdata.com>
In-Reply-To: <d4a15deb-8a85-b78c-b447-bc5df7c4a926@oss.nttdata.com>
From: Julien Rouhaud <rjuju123@gmail.com>
Date: Mon, 29 Jun 2020 11:17:14 +0200
Message-ID: 
 <CAOBaU_awkSmyBxUXZLfH0DrogqeXd6W8TBmLpF7f--RrxPRy9A@mail.gmail.com>
Subject: Re: track_planning causing performance regression
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: "Tharakan, Robins" <tharar@amazon.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jun 29, 2020 at 10:55 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
>
> On 2020/06/29 16:05, Julien Rouhaud wrote:
> > On Mon, Jun 29, 2020 at 7:49 AM Tharakan, Robins <tharar@amazon.com> wrote:
> >>
> >> During fully-cached SELECT-only test using pgbench, Postgres v13Beta1 shows
>
> Thanks for the benchmark!
>
>
> >> ~45% performance drop [2] at high DB connection counts (when compared with v12.3)
>
> That's bad :(
>
>
> >>
> >> Disabling pg_stat_statements.track_planning (which is 'On' by default)
> >> brings the TPS numbers up to v12.3 levels.
> >>
> >> The inflection point (in this test-case) is 128 Connections, beyond which the
> >> TPS numbers are consistently low. Looking at the mailing list [1], this issue
> >> didn't surface earlier possibly since the regression is trivial at low connection counts.
> >>
> >> It would be great if this could be optimized further, or track_planning
> >> disabled (by default) so as to not trip users upgrading from v12 with pg_stat_statement
> >> enabled (but otherwise not particularly interested in track_planning).
>
> 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?
It'll also quite increase the shared memory consumption.

I'm wondering if we could instead use atomics to store the counters.
The only downside is that we won't guarantee per-row consistency
anymore, which may be problematic.