Обсуждение: Slightly reduce BufMgrLock contention

This patch prevents btbulkdelete() from calling WriteNoReleaseBuffer()
several times for the same buffer.  Thus it saves a few
LWLockAquire(BufMgrLock, LW_EXCLUSIVE) and LWLockRelease(BufMgrLock)
calls.

Maybe we do not need a BufMgrLock at all, because we have a super
exclusive lock on the buffer?  I was not sure and decided to stay on
the safe side ...

Servus
 Manfred
diff -ruN ../base/src/backend/access/nbtree/nbtree.c src/backend/access/nbtree/nbtree.c
--- ../base/src/backend/access/nbtree/nbtree.c    2002-06-21 02:12:14.000000000 +0200
+++ src/backend/access/nbtree/nbtree.c    2002-08-30 11:44:35.000000000 +0200
@@ -615,6 +615,7 @@
     {
         Buffer        buf;
         BlockNumber lockedBlock = InvalidBlockNumber;
+        bool        dirty = false;

         /* we have the buffer pinned and locked */
         buf = so->btso_curbuf;
@@ -662,14 +663,19 @@
                     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
                     LockBufferForCleanup(buf);
                     lockedBlock = blkno;
+                    dirty = false;
                 }
                 else
                 {
                     /* Okay to delete the item from the page */
                     _bt_itemdel(rel, buf, current);

-                    /* Mark buffer dirty, but keep the lock and pin */
-                    WriteNoReleaseBuffer(buf);
+                    if (!dirty)
+                    {
+                        /* Mark buffer dirty, but keep the lock and pin */
+                        WriteNoReleaseBuffer(buf);
+                        dirty = true;
+                    }

                     tuples_removed += 1;
                 }

Manfred Koizar <mkoi-pg@aon.at> writes:
> This patch prevents btbulkdelete() from calling WriteNoReleaseBuffer()
> several times for the same buffer.  Thus it saves a few
> LWLockAquire(BufMgrLock, LW_EXCLUSIVE) and LWLockRelease(BufMgrLock)
> calls.

I don't like this patch, because it embeds into the caller the notion
that it's okay to keep changing the page *after* calling WriteBuffer.
It would stop working if we ever re-implemented the buffer manager in
a way that caused writes to appear synchronous.  I'm not sure that's
likely, but for the amount of gain here I do not think that introducing
extra coupling between btbulkdelete and bufmgr internal details is a
good idea.

What would make more sense is to tweak write_buffer to behave the way
SetBufferCommitInfoNeedsSave does, thus buying a similar speedup
globally instead of only for this one caller.

(In fact, since SetBufferCommitInfoNeedsSave is not really different
anymore from WriteNoReleaseBuffer, you could then just reimplement it as
write_buffer(buf, false).  I don't want to eliminate the routine,
because I think it's good for callers to make the distinction between
data writes and commit-bit updates, but there's no reason for bufmgr to
contain duplicate code.)

            regards, tom lane

On Fri, 30 Aug 2002 10:12:34 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:
>I don't like this patch, because it embeds into the caller the notion
>that it's okay to keep changing the page *after* calling WriteBuffer.

Now, after you made that clear, I don't like the patch either.  Forget
it.  Thanks for the review.

>What would make more sense is to tweak write_buffer [...]

True, but I'll not touch this area for 7.3 now - two days before beta.

Servus
 Manfred