On Fri, Dec 10, 2010 at 8:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> I think the first patch (relpersistence-v4.patch) is ready to commit,
> and the third patch to allow synchronous commits to become
> asynchronous when it doesn't matter (relax-sync-commit-v1.patch)
> doesn't seem to be changing much either, although I would appreciate
> it if someone with more expertise than I have with our write-ahead
> logging system would give it a quick once-over.
I don't understand what the point of the relax-sync-commit patch is.
If XactLastRecEnd.xrecoff == 0, then calling
XLogFlush(XactLastRecEnd) is pretty much a null operation anyway
because it will short-circuit at the early statement:
if (XLByteLE(record, LogwrtResult.Flush)) return
Or at least it had better return at that point, or we might have a
serious problem. If XactLastRecEnd.xrecoff == 0 then the only way to
keep going is if XactLastRecEnd.xlogid is ahead of
LogwrtResult.Flush.xlogid.
I guess that could happen legitimately if the logs have recently
rolled over the 4GB boundary, and XactLastRecEnd is aware of this
while LogwrtResult is not yet aware of it. I don't know if that is a
possible state of affairs. If it is, then the result would be that on
very rare occasion your patch removes a spurious, but not harmful
other than performance, fsync.
If somehow XactLastRecEnd gets a falsely advanced value of xlogid,
then calling XLogFlush with it would cause a PANIC "xlog write request
%X/%X is past end of log %X/%X". So unless people have been seeing
this, that must not be able to happen. And looking at the only places
XactLastRecEnd.xlogid get set, I don't see how it could happen.
So maybe in your patch:
if ((wrote_xlog && XactSyncCommit) || forceSyncCommit || nrels > 0)
should be
if (wrote_xlog && (XactSyncCommit || forceSyncCommit || nrels > 0) )
It seems like on general principles we should not be passing to
XLogFlush a structure which is by definition invalid.
But even if XLogFlush is going to return immediately, that doesn't
negate the harm caused by commit_delay doing its thing needlessly.
Perhaps that was the original motivation for your patch.
Cheers,
Jeff