Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Updated patches attached.
I've gotten through several days of performance tests for this pair
of related patches, with results posted on a separate thread. I'll
link those in to the CF application shortly. To summarize the other
(fairly long) thread on benchmarks, it seemed like there might be a
very slight slowdown at low concurrency, but it could be the random
alignment of code with and without the patch; it was a small enough
fraction of a percent to be negligible, in my opinion. At higher
concurrency levels the patch showed significant performance
improvements. Besides the overall improvement in the median tps
numbers of several percent, there was significant mitigation of the
"performance collapse" phenomenon, where some runs were much slower
than others. It seems a clear win in terms of performance.
I've gotten through code review of the flexlock-v2.patch, and have
decided to post on that before I go through the
procarraylock-v1.patch code.
Not surprisingly, this patch was in good form and applied cleanly.
There were doc changes, and I can't see where any changes to the
tests are required. I liked the structure, and only found a few
nit-picky things to point out:
I didn't see why num_held_flexlocks and held_flexlocks had the
static keyword removed from their declarations.
FlexLockRemember() seems to have a pasto for a comment. Maybe
change to something like: "Add lock to list of locks held by this
backend."
In procarraylock.c there is this:
/* If there are no lockers, clar the critical PGPROC fields. */
s/clar/clear/
I have to admit I don't have my head around the extraWaits issue, so
I can't personally vouch for that code, although I have no reason to
doubt it, either. Everything else was something that I at least
*think* I understand, and it looked good to me.
I'm not changing the status until I get through the other patch
file, which should be tomorrow.
-Kevin