BufferSync() performance

Hello,

Bruce Momjian is with us for some training sessions this week and has
asked me to post the following question to the mailing list (I have chosen
this list as it was suggested not to post directly to hackers list):

When looking at function BufferSync() in
postgresql/src/backend/storage/buffer/bufmgr.c we found that all buffers
are checked and BM_CHECKPOINT_NEEDED is set for the ones which are dirty.

Strangely, a lock is acquired _before_ doing the check. I believe this
might cause an unnecessary loss of performance (e.g. if 90% of all buffers
are not dirty).

I think this could be realized more efficient using an unlocked check
first, followed by a second (locked) one in case the page appears dirty.
This would cost one additional integer comparison in case a page is dirty,
but save a lot of lock cycles (see patch below).

Would this work or is there a special reason why the original check was
done with lock held?

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index bd053d5..4bdc2bd 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1157,15 +1157,16 @@ BufferSync(int flags)
  		 * Header spinlock is enough to examine BM_DIRTY, see comment in
  		 * SyncOneBuffer.
  		 */
-		LockBufHdr(bufHdr);
-
  		if (bufHdr->flags & BM_DIRTY)
  		{
-			bufHdr->flags |= BM_CHECKPOINT_NEEDED;
-			num_to_write++;
+			LockBufHdr(bufHdr);
+			if (bufHdr->flags & BM_DIRTY)
+			{
+				bufHdr->flags |= BM_CHECKPOINT_NEEDED;
+				num_to_write++;
+			}
+			UnlockBufHdr(bufHdr);
  		}
-
-		UnlockBufHdr(bufHdr);
  	}

if (num_to_write == 0)

Regards

Guido Ostkamp