Обсуждение: Re: [PERFORM] In progress INSERT wrecks plans on table
(Cc: to pgsql-performance dropped, pgsql-hackers added.) At 2013-05-06 09:14:01 +0100, simon@2ndQuadrant.com wrote: > > New version of patch attached which fixes a few bugs. I read the patch, but only skimmed the earlier discussion about it. In isolation, I can say that the patch applies cleanly and looks sensible for what it does (i.e., cache pgprocno to speed up repeated calls to TransactionIdIsInProgress(somexid)). In that sense, it's ready for committer, but I don't know if there's a better/more complete/etc. way to address the original problem. -- Abhijit
On 06/23/2013 09:43 PM, Abhijit Menon-Sen wrote: > (Cc: to pgsql-performance dropped, pgsql-hackers added.) > > At 2013-05-06 09:14:01 +0100, simon@2ndQuadrant.com wrote: >> >> New version of patch attached which fixes a few bugs. > > I read the patch, but only skimmed the earlier discussion about it. In > isolation, I can say that the patch applies cleanly and looks sensible > for what it does (i.e., cache pgprocno to speed up repeated calls to > TransactionIdIsInProgress(somexid)). > > In that sense, it's ready for committer, but I don't know if there's a > better/more complete/etc. way to address the original problem. Has this patch had performance testing? Because of the list crossover I don't have any information on that. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 07/08/2013 10:11 AM, Josh Berkus wrote: > On 06/23/2013 09:43 PM, Abhijit Menon-Sen wrote: >> (Cc: to pgsql-performance dropped, pgsql-hackers added.) >> >> At 2013-05-06 09:14:01 +0100, simon@2ndQuadrant.com wrote: >>> >>> New version of patch attached which fixes a few bugs. >> >> I read the patch, but only skimmed the earlier discussion about it. In >> isolation, I can say that the patch applies cleanly and looks sensible >> for what it does (i.e., cache pgprocno to speed up repeated calls to >> TransactionIdIsInProgress(somexid)). >> >> In that sense, it's ready for committer, but I don't know if there's a >> better/more complete/etc. way to address the original problem. > > Has this patch had performance testing? Because of the list crossover I > don't have any information on that. Due to the apparent lack of performance testing, I'm setting this back to "needs review". -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
At 2013-07-10 09:47:34 -0700, josh@agliodbs.com wrote: > > Due to the apparent lack of performance testing, I'm setting this back > to "needs review". The original submission (i.e. the message linked from the CF page) includes test results that showed a clear performance improvement. Here's an excerpt: > OK, here's a easily reproducible test... > > Prep: > DROP TABLE IF EXISTS plan; > CREATE TABLE plan > ( > id INTEGER NOT NULL, > typ INTEGER NOT NULL, > dat TIMESTAMP, > val TEXT NOT NULL > ); > insert into plan select generate_series(1,100000), 0, > current_timestamp, 'some texts'; > CREATE UNIQUE INDEX plan_id ON plan(id); > CREATE INDEX plan_dat ON plan(dat); > > testcase.pgb > select count(*) from plan where dat is null and typ = 3; > > Session 1: > pgbench -n -f testcase.pgb -t 100 > > Session 2: > BEGIN; insert into plan select 1000000 + generate_series(1, 100000), > 3, NULL, 'b'; > > Transaction rate in Session 1: (in tps) > (a) before we run Session 2: > Current: 5600tps > Patched: 5600tps > > (b) after Session 2 has run, yet before transaction end > Current: 56tps > Patched: 65tps > > (c ) after Session 2 has aborted > Current/Patched: 836, 1028, 5400tps > VACUUM improves timing again > > New version of patch attached which fixes a few bugs. > > Patch works and improves things, but we're still swamped by the block > accesses via the index. -- Abhijit
On 07/10/2013 10:09 PM, Abhijit Menon-Sen wrote: > At 2013-07-10 09:47:34 -0700, josh@agliodbs.com wrote: >> >> Due to the apparent lack of performance testing, I'm setting this back >> to "needs review". > > The original submission (i.e. the message linked from the CF page) > includes test results that showed a clear performance improvement. > Here's an excerpt: I didn't see that, and nobody replied to my email. So, where are we with this patch, then? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
At 2013-07-11 17:47:58 -0700, josh@agliodbs.com wrote: > > So, where are we with this patch, then? It's ready for committer. -- Abhijit
On Wed, Jul 10, 2013 at 10:09 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote: > At 2013-07-10 09:47:34 -0700, josh@agliodbs.com wrote: >> >> Due to the apparent lack of performance testing, I'm setting this back >> to "needs review". > > The original submission (i.e. the message linked from the CF page) > includes test results that showed a clear performance improvement. I think the reviewer of a performance patch should do some independent testing of the performance, to replicate the author's numbers; and hopefully with a few different scenarios. Cheers, Jeff
At 2013-07-12 16:25:14 -0700, jeff.janes@gmail.com wrote:
>
> I think the reviewer of a performance patch should do some independent
> testing of the performance, to replicate the author's numbers; and
> hopefully with a few different scenarios.
You're quite right. I apologise for being lazy; doubly so because I
can't actually see any difference while running the test case with
the patches applied.
unpatched: before: 1629.831391, 1559.793758, 1498.765018, 1639.384038 during: 37.434492, 37.044989,
37.112422, 36.950895 after : 46.591688, 46.341256, 46.042169, 46.260684
patched: before: 1813.091975, 1798.923524, 1629.301356, 1606.849033 during: 37.344987, 37.207359, 37.406788,
37.316925 after : 46.657747, 46.537420, 46.746377, 46.577052
("before" is before starting session 2; "during" is after session 2
inserts, but before it commits; "after" is after session 2 issues a
rollback.)
The timings above are with both xid_in_snapshot_cache.v1.patch and
cache_TransactionIdInProgress.v2.patch applied, but the numbers are
not noticeably different with only the first patch applied. After I
"vacuum plan", the timings in both cases return to normal.
In a quick test with gdb (and also in perf report output), I didn't see
the following block in procarray.c being entered at all:
+ if (max_prepared_xacts == 0 && pgprocno >= 0 &&
+ (TransactionIdEquals(xid, pxid) || TransactionIdEquals(xid, cxid)))
+ { …
I'll keep looking, but comments are welcome. I'm setting this back to
"Needs Review" in the meantime.
-- Abhijit
At 2013-07-13 14:19:23 +0530, ams@2ndQuadrant.com wrote: > > The timings above are with both xid_in_snapshot_cache.v1.patch and > cache_TransactionIdInProgress.v2.patch applied For anyone who wants to try to reproduce the results, here's the patch I used, which is both patches above plus some typo fixes in comments. -- Abhijit