Re: Why do we still have commit_delay and commit_siblings?

On Sun, May 13, 2012 at 4:17 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> This code is our pre-9.2 group commit implementation, pretty much in
> its entirety:
>
> if (CommitDelay > 0 && enableFsync &&
>        MinimumActiveBackends(CommitSiblings))
>        pg_usleep(CommitDelay);

A semantic issue, I guess, but I would say that all the code that
keeps the xlogctl->LogwrtRqst updated and reads from it is also part
of the group commit implementation.

>
> This code is placed directly before the RecordTransactionCommit() call
> of XLogFlush(). It seeks to create a delay of commit_delay immediately
> prior to flushing when MinimumActiveBackends(CommitSiblings), in the
> hope that that delay will be enough that when we do eventually reach
> XLogFlush(), we can fastpath out of it due to Postgres by then having
> already flushed up to the XLogRecPtr we need flushed.

I think it is more useful to put it the other way around.  The hope is
that when we eventually reach XLogFlush, we will find that other
people have added their commit records, so that we can fsync theirs as
well as our, letting them fast path out later on.

Really if someone else is already doing the sleep, there is no reason
for the current process to do so as well.  All that will do is delay
the time before the current process wakes up and realizes it has
already been fsynced.  Instead it should block on the other sleeping
process.  But that didn't help much if any when I tried it a couple
years ago.  It might work better now.
...

> The original group commit patch that Simon and I submitted deprecated
> these settings, and I fail to understand why the committed patch
> didn't do that too. These days, the only way this code could possibly
> result in a fastpath is here, at transam/xlog.c:2094, with the
> potentially stale (and likely too stale to be useful when new group
> commit is in play) LogwrtResult value:
>
>                /* done already? */
>                if (XLByteLE(record, LogwrtResult.Flush))
>                        break;

Why is that likely too stale to be useful?  It was updated just 4
lines previous.


>
> Now, previously, we could also fastpath a bit later, at about
> transam/xlog.c:2114:
>
>                /* Got the lock */
>                LogwrtResult = XLogCtl->LogwrtResult;
>                if (!XLByteLE(record, LogwrtResult.Flush))
>                {
>                        .....
>                }
>                /* control might have fastpathed and missed the above block. We're
> done now. */
>
> ...but now control won't even reach here unless we have the exclusive
> lock on WALWriteLock (i.e. we're the leader backend during 9.2's group
> commit), so fastpathing out of XLogFlush() becomes very rare in 9.2 .
>
> One illuminating way of quickly explaining the new group commit code
> is that it also inserts a delay at approximately the same place (well,
> more places now, since the delay was previously inserted only at the
> xact.c callsite of XLogFlush(), and there are plenty more sites than
> that), only that delay is now just about perfectly adaptive. That
> isn't quite the full truth, since we *also* have the benefit of
> *knowing* that there is an active leader/flusher at all times we're
> delayed.

I don't get what you are saying.  The new code does not have any more
delays than the old code did, there is still only one
pg_usleep(CommitDelay), and it is still in the same place.

Cheers,

Jeff