Re: Draft release notes complete - Mailing list pgsql-hackers

On Mon, May 14, 2012 at 9:06 AM, Bruce Momjian <bruce@momjian.us> wrote:
> On Sun, May 13, 2012 at 09:01:03PM +0100, Peter Geoghegan wrote:
>> On 12 May 2012 01:37, Robert Haas <robertmhaas@gmail.com> wrote:
>> > Right.  It's not a new feature; it's a performance improvement.  We've
>> > had group commit for a long time; it just didn't work very well
>> > before.  And it's not batching the commits better; it's reducing the
>> > lock contention around realizing that the batched commit has happened.
>>
>> This understanding of group commit is technically accurate, but the
>> practical implications of the new code are that lots of people benefit
>> from group commit, potentially to rather a large degree, where before
>> they had exactly no benefit from group commit. We never officially
>> called group commit group commit outside of git commit messages
>> before. Therefore, it is sort of like we didn't have group commit
>> before but now we do, and it's an implementation that's probably as
>> effective as that of any of our competitors. It is for that reason
>> that I suggested group commit get a more prominent billing, and that
>> it actually be officially referred to as group commit. I'm glad that
>> the release notes now actually refer to group commit.
>>
>> Now, I realise that as one of the authors of the feature I am open to
>> accusations of lacking objectivity - clearly it isn't really my place
>> to try and influence the feature's placement, and this is the last I
>> will say on the matter unless someone else brings it up again. I just
>> think that pedantically characterising this as an improvement to our
>> existing group commit implementation within a document that will be
>> read like a press release is a bad decision, especially since our
>> competitors never had a group commit implementation that was far
>> inferior to their current implementation. The assumption will be that
>> it's a small improvement that's hardly worth noticing at all.
>
> Thanks for the summary. I know we talk about group commit, but I wasn't
> aware that it had not been exposed to our general users.  I agree we
> need to reword the item as you suggested.  So this group commit happens
> even if users don't change these?
>
>        #commit_delay = 0           # range 0-100000, in microseconds
>        #commit_siblings = 5            # range 1-1000

If a bunch of people are standing around waiting for a door to unlock
and they are mutually aware of each other, it has never been the case
(or at least not for years) that the first person through the door
would systematically slam it in everyone else's face.  Is this enough
to qualify as "group commit"?  If so, group commit has "always"
(again, at least for years) been there.  The new code simply makes it
less likely that the group will trip over each others feet as they all
stream through the door.

The commit_delay settings cover the case where the door unlocks, and
you open it, but then perhaps you stand there for an a few minutes
holding it open in case someone else happens to show up.  This is
pretty much orthogonal to the prior case.  You can not wait for new
people to show up, but trip over the feet of the people already there.Or you can wait for new people to show up, then
tripover them.  Or 
not trip over them, with or without waiting for new arrival.

(For the analogy to work, this particular door refuses to unlock more
than once every 5 minutes.  Maybe it is for a very slow elevator)

> This is the git commit message:
>
>    Make group commit more effective.
>
>    When a backend needs to flush the WAL, and someone else is already flushing
>    the WAL, wait until it releases the WALInsertLock and check if we still need
>    to do the flush or if the other backend already did the work for us, before
>    acquiring WALInsertLock. This helps group commit, because when the WAL flush
>    finishes, all the backends that were waiting for it can be woken up in one
>    go, and the can all concurrently observe that they're done, rather than
>    waking them up one by one in a cascading fashion.
>
>    This is based on a new LWLock function, LWLockWaitUntilFree(), which has
>    peculiar semantics. If the lock is immediately free, it grabs the lock and
>    returns true. If it's not free, it waits until it is released, but then
>    returns false without grabbing the lock. This is used in XLogFlush(), so
>    that when the lock is acquired, the backend flushes the WAL, but if it's
>    not, the backend first checks the current flush location before retrying.
>
>    Original patch and benchmarking by Peter Geoghegan and Simon Riggs, although
>    this patch as committed ended up being very different from that.
>
>    (Heikki Linnakangas)
>
> Is that commit message inaccurate?

I think the commit message is accurate, other than saying
WALInsertLock where it meant WALWriteLock.

Cheers,

Jeff