On Mon, Nov 7, 2011 at 7:28 AM, Robert Haas <robertmh...@gmail.com> wrote: > On Mon, Nov 7, 2011 at 6:45 AM, Heikki Linnakangas > <heikki.linnakan...@enterprisedb.com> wrote: >> While looking at GetSnapshotData(), it occurred to me that when a PGPROC >> entry does not have its xid set, ie. xid == InvalidTransactionId, it's >> pointless to check the subxid array for that proc. If a transaction has no >> xid, none of its subtransactions can have an xid either. That's a trivial >> optimization, see attached. I tested this with "pgbench -S -M prepared -c >> 500" on the 8-core box, and it seemed to make a 1-2% difference (without the >> other patch). So, almost in the noise, but it also doesn't cost us anything >> in terms of readability or otherwise. > > Oh, that's a good idea. +1 for doing that now, and then we can keep > working on the rest of it.
I spent some time looking at (and benchmarking) this idea today. I rearranged that section of code a little more into what seemed like the optimal order to avoid doing more work than necessary, but I wasn't getting much of a gain out of it even on unlogged tables and on permanent tables it was looking like a small loss, though I didn't bother letting the benchmarks run for long enough to nail that down because it didn't seem to amount to much one way or the other. I added then added one more change that helped quite a lot: I introduced a macro NormalTransactionIdPrecedes() which works like TransactionIdPrecdes() but (a) is only guaranteed to work if the arguments are known to be normal transaction IDs (which it also asserts, for safety) and (b) is a macro rather than a function call. I found three places to use that inside GetSnapshotData(), and the results with that change look fairly promising. Nate Boley's box, m = master, s = with the attached patch, median of three 5-minute runs at scale factor 100, config same as my other recent tests: Permanent Tables m01 tps = 617.734793 (including connections establishing) s01 tps = 620.330099 (including connections establishing) m08 tps = 4589.566969 (including connections establishing) s08 tps = 4545.942588 (including connections establishing) m16 tps = 7618.842377 (including connections establishing) s16 tps = 7596.759619 (including connections establishing) m24 tps = 11770.534809 (including connections establishing) s24 tps = 11789.424152 (including connections establishing) m32 tps = 10776.660575 (including connections establishing) s32 tps = 10643.361817 (including connections establishing) m80 tps = 11057.353339 (including connections establishing) s80 tps = 10598.254713 (including connections establishing) Unlogged Tables m01 tps = 668.145495 (including connections establishing) s01 tps = 676.793337 (including connections establishing) m08 tps = 4715.214745 (including connections establishing) s08 tps = 4737.833913 (including connections establishing) m16 tps = 8110.607784 (including connections establishing) s16 tps = 8192.013414 (including connections establishing) m24 tps = 14120.753841 (including connections establishing) s24 tps = 14302.915040 (including connections establishing) m32 tps = 17886.032656 (including connections establishing) s32 tps = 18735.319468 (including connections establishing) m80 tps = 12711.930739 (including connections establishing) s80 tps = 17592.715471 (including connections establishing) Now, you might not initially find those numbers all that encouraging. Of course, the unlogged tables numbers are quite good, especially at 32 and 80 clients, where the gains are quite marked. But the permanent table numbers are at best a wash, and maybe a small loss. Interestingly enough, some recent benchmarking of the FlexLocks patch showed a similar (though more pronounced) effect: http://archives.postgresql.org/pgsql-hackers/2011-12/msg00095.php Now, both the FlexLocks patch and this patch aim to mitigate contention problems around ProcArrayLock. But they do it in completely different ways. When I got a small regression on permanent tables with the FlexLocks patch, I thought the problem was with the patch itself, which is believable, since it was tinkering with the LWLock machinery, a fairly global sort of change that you can well imagine might break something. But it's hard to imagine how that could possibly be the case here, especially given the speedups on unlogged tables, because this patch is dead simple and entirely isolated to GetSnapshotData(). So I have a new theory: on permanent tables, *anything* that reduces ProcArrayLock contention causes an approximately equal increase in WALInsertLock contention (or maybe some other lock), and in some cases that increase in contention elsewhere can cost more than the amount we're saving here. On that theory, I'm inclined to think that's not really a problem. We'll go nuts if we refuse to commit anything until it shows a meaningful win on every imaginable workload, and it seems like this can't really be worse than the status quo; any case where it is must be some kind of artifact. We're better of getting rid of as much ProcArrayLock contention as possible, rather than keeping it around because there are corner cases where it decreases contention on some other lock. One small detail I'm noticing on further review is that I've slightly changed the semantics here: if (!TransactionIdIsNormal(xid) || NormalTransactionIdPrecedes(xmax, xid)) continue; That really ought to be testing <=, not just <. That doesn't seem like it should affect correctness, though: at worst, we unnecessarily include one or more XIDs in the snapshot that will be ignored anyway. I'll fix that and rerun the tests but I don't think it'll make any difference. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
getsnapshotdata-tweaks.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
