Обсуждение: 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

Guido Ostkamp <postgresql@ostkamp.fastmail.fm> writes:
> Would this work or is there a special reason why the original check was
> done with lock held?

This will fail, very nastily, on multiple-CPU machines with weak memory
ordering guarantees.  You can't assume you are seeing an up-to-date
value of the flag bit if you don't take the spinlock first.

There are places where we can get away with such things because a
slightly stale answer is okay, but not in BufferSync().  Failing to
include a dirty page in the checkpoint is fatal.

            regards, tom lane

On Thu, 5 Mar 2009, Guido Ostkamp wrote:

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

http://en.wikipedia.org/wiki/Race_condition

Until you have a lock on a buffer header, you can't trust that you're even
seeing consistent information about it.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD