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

On Wed, May 30, 2012 at 4:36 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> When I read this the first time, I was in full agreement.
>
> On closer inspection neither point is valid, though both points were
> worth considering.
>
>> Well, consider the one in the background writer, for example.  That's
>> just a periodic flush, so I see no benefit in having it acquire the
>> lock and then wait some more.  It already did wait.
>
> We use XLogBackgroundFlush() not XLogFlush() from background processes.
>
>> And what about
>> the case where we're flushing while holding WALInsertLock because the
>> buffer's full?  Clearly waiting is useless in that case - nobody can
>> join the group commit for exactly the same reason that we're doing the
>> flush in the first place: no buffer space.
>
> We don't flush WAL in that case, we just write it.

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.

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.

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.

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