Re: Uh, I change my mind about commit_delay + commit_siblings (sort of)

On 30 May 2012 13:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> OK, but there are a lot of places where we call XLogFlush(), and it's
> far from obvious that it's a win to do this in all of those cases.  At
> least, nobody's done that analysis.  XLogFlush is called from:
>
> - WriteTruncateXlogRec(), which is called from TruncateCLOG()
> - SlruPhysicalWritePage()
> - EndPrepare()
> - RecordTransactionCommitPrepared()
> - RecordTransactionCommit()
> - xact_redo_commit_internal()
> - CreateCheckPoint()
> - RelationTruncate()
> - FlushBuffer()
> - write_relmap_file()
>
> Most of those actually do look like reasonable places to try to get
> grouped flushing behavior, but:
>
> 1. It seems wrong to do it in xact_redo_commit_internal().  It won't
> matter if commit_siblings>0 since there won't be any other backends
> with transaction IDs anyway, but if commit_siblings==0 then we'll
> sleep for no possible benefit.

Agreed

> 2. Doing it in FlushBuffer() seems slightly iffy since we might be
> sitting on a buffer lock.  But maybe it's a win anyway, or just not
> worth worrying about.

Agreed.

The remaining cases aren't worth worrying about, apart from
SlruPhysicalWritePage() which happens during visibility checks and
needs to happen as quickly as possible also.

I would say the additional contention from waiting outweighs the
benefit of the wait in those 3 places, so skipping the wait is wise.

Should be implemented as

#define XLogFlush(lsn) XLogFlushInternal(lsn, true)
#define XLogFlushNoWait(lsn) XLogFlushInternal(lsn, false)

so we can drop the no wait version in without touching the other call points

> Another thing to think about is that if we do this across the board
> rather than just for commits, then commit_delay and commit_siblings
> will really be totally misnamed - they really ought to be flush_delay
> and flush_siblings at that point.

I think if we were to rename the parameters, then they should be called

group_commit_delay
group_commit_siblings

Since "Group Commit" is the accepted term for this behaviour, even
though, as you point out that the behaviour isn't just about commit.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services