Re: XLogInsert

Tom Lane wrote:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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.uz8sfkxycke6l8@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
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com