Tom Lane wrote:
Jaime Casanova <jcasa...@systemguards.com.ec> writes:
i haven't made any performance tests but it should gain something :),
maybe someone can make those tests?
The argument for changing this at all is only that it will improve
performance, so I'd like some independent confirmation that it does.
I've done a little review of this myself, and I'm not quite happy with how this patch was
delivered to us. The bar for committing something that touches the WAL is really
high--it needs to be a unquestionable win to touch that code. The justification of
"the patch makes the overall code a bit cleaner" is a hard sell on something
that's hard to debug (triggering bad WAL situations at will isn't easy) and critical to
the system. If there's a clear performance improvement, that helps justify why it's
worth working on. Here's the original performance justification:
"Using the only XLogInsert-bound test case I know of, parallel COPY into a skinny,
unindexed table, using 8 parallel copies on a 4 x dual-core x86_64 and with fsync turned
off (to approxiamately simulate SSD, which I do not have), I get a speed improvement of
2-4% with the patch over unpatched head."
That makes sense, and using this spec I could probably come up with the test
program to reproduce this. But I'm getting tired of doing that. It's hard
enough to reproduce performance changes when someone gives the exact
configuration and test program they used. If we're working with a verbal spec
for how to reproduce the issues, forget about it--that's more than we can
expect a reviewer to handle, and the odds of that whole thing ending well are
low.
Jeff: before we do anything else with your patch, I'd like to see a script of
some sort that runs the test you describe above, everything changed in the
postgresql.conf from the defaults, and the resulting raw numbers that come from
it on your system that prove an improvement there--not just a summary of the
change. That's really mandatory for a performance patch. If any reviewer
who's interested can't just run something and get a report suggesting whether
the patch helped or harmed results in five minutes, unless we really, really
want your patch it's just going to stall at that point. And unfortunately, in
the case of something that touches the WAL path, we really don't want to change
anything unless there's a quite good reason to do so.
I've also realized that "Patch LWlocks instrumentation" at
http://archives.postgresql.org/message-id/op.uz8sfkxycke...@soyouz should have been
evaluated as its own patch altogether. I think that the test program you're suggesting
also proves its utility though, so for now I'll keep them roped together.
Sorry this ended up so late in this CommitFest, just a series of unexpected
stuff rippled down to you. On the bright side, had you submitted this before
the whole organized CF process started, you could have waited months longer to
get the same feedback.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
g...@2ndquadrant.com www.2ndQuadrant.com
