Re: Uh, I change my mind about commit_delay + commit_siblings (sort of)

On Sat, Jun 2, 2012 at 10:44 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Tom Lane  wrote:
>> Simon Riggs  writes:
>>> On 31 May 2012 15:00, Tom Lane  wrote:
>>>> If we want to finish the beta cycle in a reasonable time period
>>>> and get back to actual development, we have to refrain from
>>>> adding more possibly-destabilizing development work to 9.2. And
>>>> that is what this is.
>>
>>> In what way is it possibly destabilising?
>>
>> I'm prepared to believe that it only affects performance, but it
>> could be destabilizing to that. It needs proper review and testing,
>> and the next CF is the right environment for that to happen.
>
> +1
>
> This is not a bug fix or even a fix for a performance regression.
> The train has left the station; the next one will be along shortly.

And here it is. There are a couple of outstanding issues here upon
which it would be helpful to get input from a few more people:

1. Are there any call sites from which this should be disabled, either
because the optimization won't help, or because the caller is holding
a lock that we don't want them sitting on for a moment longer than
necessary? I think the worst case is that we're doing something like
splitting the root page of a btree, and now nobody can walk the btree
until the flush finishes, and here we are doing an "unnecessary"
sleep. I am not sure where I can construct a workload where this is a
real as opposed to a theoretical problem, though. So maybe we should
just not worry about it. It suffices for this to be an improvement
over the status quo; it doesn't have to be perfect. Thoughts?

2. Should we rename the GUCs, since this patch will cause them to
control WAL flush in general, as opposed to commit specifically?
Peter Geoghegan and Simon were arguing that we should retitle it to
group_commit_delay rather than just commit_delay, but that doesn't
seem to be much of an improvement in describing what the new behavior
will actually be, and I am doubtful that it is worth creating a naming
incompatibility with previous releases for a cosmetic change. I
suggested wal_flush_delay, but there's no consensus on that.
Opinions?

Also, I think I see a straightforward, if minor, improvement. Right
now, the patch has this:

* Sleep before flush! By adding a delay here, we may give further
* backends the opportunity to join the backlog of group commit
* followers; this can significantly improve transaction throughput, at
* the risk of increasing transaction latency.
*
* We do not sleep if enableFsync is not turned on, nor if there are
* fewer than CommitSiblings other backends with active transactions.
*/
if (CommitDelay > 0 && enableFsync &&
MinimumActiveBackends(CommitSiblings))
pg_usleep(CommitDelay);

/* Got the lock */
LogwrtResult = XLogCtl->LogwrtResult;
if (!XLByteLE(record, LogwrtResult.Flush))
{
/* try to write/flush later additions to XLOG as well */
if (LWLockConditionalAcquire(WALInsertLock, LW_EXCLUSIVE))

The XLByteLE test four lines from the bottom should happen before we
consider whether to sleep, because it's possible that we'll discover
that our flush request has already been satisfied and exit without
doing anything, in which case the sleep is a waste. The attached
version adjusts the logic accordingly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company