Re: unlogged tables

On Fri, Dec 10, 2010 at 8:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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