Re: Group commit and commit delay/siblings

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


diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 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.c
index 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.c
index 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.c
index 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.h
index 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);