Re: XLogInsert - Mailing list pgsql-hackers

Tom Lane wrote: <blockquote cite="mid:1524.1260369585@sss.pgh.pa.us" type="cite"><pre wrap="">Jaime Casanova <a
class="moz-txt-link-rfc2396E"href="mailto:jcasanov@systemguards.com.ec"><jcasanov@systemguards.com.ec></a>
writes:</pre><blockquote type="cite"><pre wrap="">i haven't made any performance tests but it should gain something
:),
maybe someone can make those tests?   </pre></blockquote><pre wrap="">
The argument for changing this at all is only that it will improve
performance, so I'd like some independent confirmation that it does.</pre></blockquote><pre wrap="">I've done a little
reviewof this myself, and I'm not quite happy with how this patch was delivered to us.  The bar for committing
somethingthat touches the WAL is really high--it needs to be a unquestionable win to touch that code.  The
justificationof "the patch makes the overall code a bit cleaner" is a hard sell on something that's hard to debug
(triggeringbad 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
copieson a 4 x dual-core x86_64 and with fsync turned off (to approxiamately simulate SSD, which I do not have), I get
aspeed 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
gettingtired of doing that.  It's hard enough to reproduce performance changes when someone gives the exact
configurationand test program they used.  If we're working with a verbal spec for how to reproduce the issues, forget
aboutit--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
describeabove, everything changed in the postgresql.conf from the defaults, and the resulting raw numbers that come
fromit on your system that prove an improvement there--not just a summary of the change.  That's really mandatory for a
performancepatch.  If any reviewer who's interested can't just run something and get a report suggesting whether the
patchhelped or harmed results in five minutes, unless we really, really want your patch it's just going to stall at
thatpoint.  And unfortunately, in the case of something that touches the WAL path, we really don't want to change
anythingunless there's a quite good reason to do so.
 

I've also realized that "Patch LWlocks instrumentation" at <a class="moz-txt-link-freetext"
href="http://archives.postgresql.org/message-id/op.uz8sfkxycke6l8@soyouz">http://archives.postgresql.org/message-id/op.uz8sfkxycke6l8@soyouz</a>
shouldhave been evaluated as its own patch altogether.  I think that the test program you're suggesting also proves its
utilitythough, 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
thesame feedback.
 

</pre> <pre class="moz-signature" cols="72">-- 
Greg Smith    2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
<a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a>  <a
class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.com">www.2ndQuadrant.com</a>
 
</pre>