From: Greg Smith <greg@2ndquadrant.com>
To: Jignesh Shah <jkshah@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Rob Wultsch <wultsch@gmail.com>
Cc: postgres performance list <pgsql-performance@postgresql.org>
Subject: Re: Group commit and commit delay/siblings
Date: Mon, 06 Dec 2010 23:52:50 -0500
Message-ID: <4CFDBDA2.4050705@2ndquadrant.com> (raw)
In-Reply-To: <AANLkTim7DE99=R6FH=5bD3yTC9t_xjVUvf+e3knbfoyK@mail.gmail.com>
References: <AANLkTik3dr076BMQ6MqCLZZ0x2-qxmQgWoqCR7c0ePoD@mail.gmail.com>
<AANLkTi=fUBnfR7K5uAQZkvaymuVHzUuCpeu4NOtLOMOU@mail.gmail.com>
<4CFC6814.9010002@2ndquadrant.com>
<28624.1291658109@sss.pgh.pa.us>
<AANLkTim7DE99=R6FH=5bD3yTC9t_xjVUvf+e3knbfoyK@mail.gmail.com>
Jignesh Shah wrote:
> On Tue, Dec 7, 2010 at 1:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> I could have sworn we'd refactored that to something like
>> bool ThereAreAtLeastNActiveBackends(int n)
>> which could drop out of the loop as soon as it'd established what we
>> really need to know...I'd suggest that we just improve the
>> coding so that we don't scan ProcArray at all when commit_siblings is 0.
>>
>> (I do agree with improving the docs to warn people away from assuming
>> this is a knob to frob mindlessly.)
>>
> In that case I propose that we support commit_siblings=0 which is not
> currently supported. Minimal value for commit_siblings is currently
> 1. If we support commit_siblings=0 then it should short-circuit that
> function call which is often what I do in my tests with commit_delay.
>
Everybody should be happy now: attached patch refactors the code to
exit as soon as the siblings count is exceeded, short-circuits with no
scanning of ProcArray if the minimum is 0, and allows setting the
siblings to 0 to enable that shortcut:
postgres# select name,setting,min_val,max_val from pg_settings where
name='commit_siblings';
name | setting | min_val | max_val
-----------------+---------+---------+---------
commit_siblings | 5 | 0 | 1000
It also makes it clear in the docs that a) group commit happens even
without this setting being touched, and b) it's unlikely to actually
help anyone. Those are the two parts that seem to confuse people
whenever this comes up. Thanks to Rob and the rest of the Facebook
commentators for helping highlight exactly what was wrong with the way
those were written. (It almost makes up for the slight distaste I get
from seeing "Greg likes MySQL at Facebook" on my Wall after joining in
that discussion)
I can't rebuild the docs on the system I wrote this on at the moment; I
hope I didn't break anything with my edits but didn't test that yet.
I'll add this into the next CommitFest so we don't forget about it, but
of course Jignesh is welcome to try this out at his convience to see if
I've kept the behavior he wants while improving its downside.
--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services and Support www.2ndQuadrant.us
Attachments:commit-delay-refactor-v1.patch (text/x-patch, 6.7K) downloaddiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgmlindex 1ca51ef..f1d3ca2 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*************** SET ENABLE_SEQSCAN TO OFF;
*** 1683,1699 ****
</indexterm>
<listitem>
<para>
! Time delay between writing a commit record to the WAL buffer
! and flushing the buffer out to disk, in microseconds. A
! nonzero delay can allow multiple transactions to be committed
! with only one <function>fsync()</function> system call, if
system load is high enough that additional transactions become
ready to commit within the given interval. But the delay is
just wasted if no other transactions become ready to
commit. Therefore, the delay is only performed if at least
<varname>commit_siblings</varname> other transactions are
active at the instant that a server process has written its
! commit record. The default is zero (no delay).
</para>
</listitem>
</varlistentry>
--- 1683,1706 ----
</indexterm>
<listitem>
<para>
! When the commit data for a transaction is flushed to disk, any
! additional commits ready at that time are also flushed out.
! <varname>commit_delay</varname> adds a time delay, set in
! microseconds, before writing some commit records to the WAL
! buffer and flushing the buffer out to disks. A nonzero delay
! can allow more transactions to be committed with only one call
! to the active <varname>wal_sync_method</varname>, if
system load is high enough that additional transactions become
ready to commit within the given interval. But the delay is
just wasted if no other transactions become ready to
commit. Therefore, the delay is only performed if at least
<varname>commit_siblings</varname> other transactions are
active at the instant that a server process has written its
! commit record. The default is zero (no delay). Since
! all pending commit data flushes are written at every flush
! regardless of this setting, it is rare that adding delay to
! that by increasing this parameter will actually improve commit
! performance.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.cindex d2e2e11..79c9c0d 100644
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*************** RecordTransactionCommit(void)
*** 1052,1058 ****
* fewer than CommitSiblings other backends with active transactions.
*/
if (CommitDelay > 0 && enableFsync &&
! CountActiveBackends() >= CommitSiblings)
pg_usleep(CommitDelay);
XLogFlush(XactLastRecEnd);
--- 1052,1058 ----
* fewer than CommitSiblings other backends with active transactions.
*/
if (CommitDelay > 0 && enableFsync &&
! MinimumActiveBackends(CommitSiblings))
pg_usleep(CommitDelay);
XLogFlush(XactLastRecEnd);
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.cindex 6e7a6db..a4114b4 100644
*** a/src/backend/storage/ipc/procarray.c
--- b/src/backend/storage/ipc/procarray.c
*************** CancelVirtualTransaction(VirtualTransact
*** 1937,1956 ****
}
/*
! * CountActiveBackends --- count backends (other than myself) that are in
! * active transactions. This is used as a heuristic to decide if
! * a pre-XLOG-flush delay is worthwhile during commit.
*
* Do not count backends that are blocked waiting for locks, since they are
* not going to get to run until someone else commits.
*/
! int
! CountActiveBackends(void)
{
ProcArrayStruct *arrayP = procArray;
int count = 0;
int index;
/*
* Note: for speed, we don't acquire ProcArrayLock. This is a little bit
* bogus, but since we are only testing fields for zero or nonzero, it
--- 1937,1961 ----
}
/*
! * MinimumActiveBackends --- count backends (other than myself) that are
! * in active transactions. Return true if the count exceeds the
! * minimum threshold passed. This is used as a heuristic to decide if
! * a pre-XLOG-flush delay is worthwhile during commit.
*
* Do not count backends that are blocked waiting for locks, since they are
* not going to get to run until someone else commits.
*/
! bool
! MinimumActiveBackends(int min)
{
ProcArrayStruct *arrayP = procArray;
int count = 0;
int index;
+ /* Quick short-circuit if no minimum is specified */+ if (min == 0)+ return true;+
/*
* Note: for speed, we don't acquire ProcArrayLock. This is a little bit
* bogus, but since we are only testing fields for zero or nonzero, it
*************** CountActiveBackends(void)
*** 1983,1991 ****
if (proc->waitLock != NULL)
continue; /* do not count if blocked on a lock */
count++;
}
! return count;
}
/*
--- 1988,1998 ----
if (proc->waitLock != NULL)
continue; /* do not count if blocked on a lock */
count++;
+ if (count >= min)+ break;
}
! return count >= min;
}
/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex dd19fe9..942acb9 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static struct config_int ConfigureNamesI
*** 1816,1822 ****
NULL
},
&CommitSiblings,
! 5, 1, 1000, NULL, NULL
},
{
--- 1816,1822 ----
NULL
},
&CommitSiblings,
! 5, 0, 1000, NULL, NULL
},
{
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.hindex 959033e..c182dcd 100644
*** a/src/include/storage/procarray.h
--- b/src/include/storage/procarray.h
*************** extern VirtualTransactionId *GetCurrentV
*** 61,67 ****
extern VirtualTransactionId *GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid);
extern pid_t CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode);
! extern int CountActiveBackends(void);
extern int CountDBBackends(Oid databaseid);
extern void CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending);
extern int CountUserBackends(Oid roleid);
--- 61,67 ----
extern VirtualTransactionId *GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid);
extern pid_t CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode);
! extern bool MinimumActiveBackends(int min);
extern int CountDBBackends(Oid databaseid);
extern void CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending);
extern int CountUserBackends(Oid roleid);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-performance@postgresql.org
Cc: greg@2ndquadrant.com, jkshah@gmail.com, tgl@sss.pgh.pa.us, wultsch@gmail.com
Subject: Re: Group commit and commit delay/siblings
In-Reply-To: <4CFDBDA2.4050705@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox