On Fri, Dec 10, 2010 at 8:16 PM, Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
