On Sat, Jun 2, 2012 at 10:44 AM, Kevin Grittner <kevin.gritt...@wicourts.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
move_delay_2012_06_27.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers