Thread: Re: [COMMITTERS] pgsql: Optimize commit_siblings in two ways to improve group commit.

Simon Riggs <simon@2ndQuadrant.com> writes:
> Optimize commit_siblings in two ways to improve group commit.
> First, avoid scanning the whole ProcArray once we know there
> are at least commit_siblings active; second, skip the check
> altogether if commit_siblings = 0.

> Greg Smith

I wonder whether we shouldn't change commit_siblings' default value to
zero while we're at it.
        regards, tom lane

On Wed, Dec 8, 2010 at 1:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Optimize commit_siblings in two ways to improve group commit.
>> First, avoid scanning the whole ProcArray once we know there
>> are at least commit_siblings active; second, skip the check
>> altogether if commit_siblings = 0.
>
>> Greg Smith
>
> I wonder whether we shouldn't change commit_siblings' default value to
> zero while we're at it.

Not that I see anything to disagree with in this patch, but what
happened to posting patches in advance of committing them?  Or did I
just miss that part?

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

Robert Haas <robertmhaas@gmail.com> writes:
> Not that I see anything to disagree with in this patch, but what
> happened to posting patches in advance of committing them?  Or did I
> just miss that part?

http://archives.postgresql.org/pgsql-performance/2010-12/msg00073.php

Possibly it should have been posted to -hackers instead, but surely you
read -performance?
        regards, tom lane

On Wed, Dec 8, 2010 at 2:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Not that I see anything to disagree with in this patch, but what
>> happened to posting patches in advance of committing them?  Or did I
>> just miss that part?
>
> http://archives.postgresql.org/pgsql-performance/2010-12/msg00073.php
>
> Possibly it should have been posted to -hackers instead, but surely you
> read -performance?

Oh, yeah I see it now.  I do read -performance, but with two orders of
magnitude more latency than -hackers.

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

Tom Lane wrote:
> http://archives.postgresql.org/pgsql-performance/2010-12/msg00073.php
>
> Possibly it should have been posted to -hackers instead, but surely you
> read -performance?
>   

Trying to figure out what exactly commit_delay and commit_siblings did 
under the hood was actually the motivation behind my first foray into 
reading the PostgreSQL source code.  Ever since, I've been annoyed that 
the behavior didn't really help the way it's intended, but was not sure 
what would be better.  The additional input from Jignesh this week on 
the performance list suddenly made it crystal clear what would preserve 
the good behavior he had seen, even improving things for his case, while 
also helping the majority who won't benefit from the commit_delay 
behavior at all a little.  I immediately wrote the patch and breathed a 
sign of relief that it was finally going to get better.

I then posted the patch and added it to the January CF.  Unbeknownst to 
me until today, Simon had the same multi-year "this itches and I can't 
make it stop" feel toward these parameters, and that's how it jumped the 
standard process.

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services and Support        www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

Greg Smith <greg@2ndquadrant.com> writes:
> I then posted the patch and added it to the January CF.  Unbeknownst to 
> me until today, Simon had the same multi-year "this itches and I can't 
> make it stop" feel toward these parameters, and that's how it jumped the 
> standard process.

I think pretty much everybody who's looked at that code had the same
feeling.  If Simon hadn't taken it, I might have.

Jignesh's explanation of what the actual usefulness of the code is
finally made sense to me: the sleep calls effectively synchronize
multiple nearby commits to happen at the next scheduler clock tick,
and then whichever one grabs the WALWriteLock first does the work.
If you've got a high enough commit volume that this is likely to
be a win, then it's unclear that taking ProcArrayLock (even shared)
to check for guys who might commit shortly is a net win.  Moreover,
it's likely that that heuristic will exclude the last-to-arrive
process who otherwise could have participated in a group flush.

I'm not entirely convinced that zero commit_siblings is a better
default than small positive values, but it's certainly plausible.
        regards, tom lane

Tom Lane wrote:
> I'm not entirely convinced that zero commit_siblings is a better
> default than small positive values, but it's certainly plausible.
>   

Not being allowed to set it to zero was certainly a limitation worth 
abolishing though; that has been the case before now, for those who 
didn't see the thread on the performance list.  I think that on the sort 
of high throughput system likely to benefit from this behavior, whether 
commit_siblings is zero or five doesn't matter very much--those people 
should cross the siblings threshold very quickly regardless.  The main 
arguments in favor of making the default lower aren't as exciting now 
that it jumps out of the loop early once finding the requisite number.

I like keeping the default at 5 though.  It keeps the person who 
experiments with increasing commit_delay from suffering when there are 
in reality not a lot of active connections.  There are essentially two 
foot-guns you have to aim before you run into the worst case here, which 
is making every single commit wait for the delay when there's really 
only one active process committing.

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services and Support        www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books