Re: Storing hot members of PGPROC out of the band

On Mon, Nov 7, 2011 at 7:28 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Nov 7, 2011 at 6:45 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)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