Re: Minor lmgr code cleanup - Mailing list pgsql-patches

On Sun, 07 Sep 2003 10:19:07 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:
>> +#define BITS_OFF(i) ~(1 << (i))
>
>I'd put another pair of parens around that.  Also, it might be worth
>moving into a header file.  There is at least one place in proc.c that
>manipulates lock masks using explicit shifts, because BITS_ON/BITS_OFF
>weren't visible outside lock.c.

Done.  Small patch attached, should be applied after the large patch.
Big fat all-in-one patch available on request.

>BTW, did you check that the code still compiles with LOCK_DEBUG enabled?

No, I didn't and it didn't. :-(  Corrected.

>How about contrib/userlock?

Unaffected by my changes, still works AFAICT.

Servus
 Manfred
diff -ruN ../base/src/backend/storage/lmgr/lock.c src/backend/storage/lmgr/lock.c
--- ../base/src/backend/storage/lmgr/lock.c    2003-09-06 23:01:05.000000000 +0200
+++ src/backend/storage/lmgr/lock.c    2003-09-07 07:53:16.000000000 +0200
@@ -111,7 +111,7 @@
              "req(%d,%d,%d,%d,%d,%d,%d)=%d "
              "grant(%d,%d,%d,%d,%d,%d,%d)=%d wait(%d) type(%s)",
              where, MAKE_OFFSET(lock),
-             lock->tag.lockmethod, lock->tag.relId, lock->tag.dbId,
+             lock->tag.lockmethodid, lock->tag.relId, lock->tag.dbId,
              lock->tag.objId.blkno, lock->grantMask,
              lock->requested[1], lock->requested[2], lock->requested[3],
              lock->requested[4], lock->requested[5], lock->requested[6],
@@ -150,12 +150,6 @@


 /*
- * These are to simplify some bit arithmetic.
- */
-#define BITS_ON(i) (1 << (i))
-#define BITS_OFF(i) ~(1 << (i))
-
-/*
  * map from lock method id to the lock table structure
  */
 static LockMethod LockMethods[MAX_LOCK_METHODS];
@@ -654,15 +648,12 @@
          * Construct bitmask of locks this process holds on this object.
          */
         {
-            int            heldLocks = 0;
-            int            tmpMask;
+            LOCKMASK        heldLocks = 0;

-            for (i = 1, tmpMask = 2;
-                 i <= lockMethodTable->numLockModes;
-                 i++, tmpMask <<= 1)
+            for (i = 1; i <= lockMethodTable->numLockModes; i++)
             {
                 if (myHolding[i] > 0)
-                    heldLocks |= tmpMask;
+                    heldLocks |= LOCKBIT_ON(i);
             }
             MyProc->heldLocks = heldLocks;
         }
@@ -725,9 +716,8 @@
                    int *myHolding)        /* myHolding[] array or NULL */
 {
     int            numLockModes = lockMethodTable->numLockModes;
-    int            bitmask;
-    int            i,
-                tmpMask;
+    LOCKMASK    bitmask;
+    int            i;
     int            localHolding[MAX_LOCKMODES];

     /*
@@ -760,11 +750,10 @@

     /* Compute mask of lock types held by other processes */
     bitmask = 0;
-    tmpMask = 2;
-    for (i = 1; i <= numLockModes; i++, tmpMask <<= 1)
+    for (i = 1; i <= numLockModes; i++)
     {
         if (lock->granted[i] != myHolding[i])
-            bitmask |= tmpMask;
+            bitmask |= LOCKBIT_ON(i);
     }

     /*
@@ -830,9 +819,9 @@
 {
     lock->nGranted++;
     lock->granted[lockmode]++;
-    lock->grantMask |= BITS_ON(lockmode);
+    lock->grantMask |= LOCKBIT_ON(lockmode);
     if (lock->granted[lockmode] == lock->requested[lockmode])
-        lock->waitMask &= BITS_OFF(lockmode);
+        lock->waitMask &= LOCKBIT_OFF(lockmode);
     LOCK_PRINT("GrantLock", lock, lockmode);
     Assert((lock->nGranted > 0) && (lock->granted[lockmode] > 0));
     Assert(lock->nGranted <= lock->nRequested);
@@ -945,7 +934,7 @@
     waitLock->requested[lockmode]--;
     /* don't forget to clear waitMask bit if appropriate */
     if (waitLock->granted[lockmode] == waitLock->requested[lockmode])
-        waitLock->waitMask &= BITS_OFF(lockmode);
+        waitLock->waitMask &= LOCKBIT_OFF(lockmode);

     /* Clean up the proc's own state */
     proc->waitLock = NULL;
@@ -1071,7 +1060,7 @@
     if (lock->granted[lockmode] == 0)
     {
         /* change the conflict mask.  No more of this lock type. */
-        lock->grantMask &= BITS_OFF(lockmode);
+        lock->grantMask &= LOCKBIT_OFF(lockmode);
     }

     LOCK_PRINT("LockRelease: updated", lock, lockmode);
@@ -1237,7 +1226,7 @@
                     lock->granted[i] -= proclock->holding[i];
                     Assert(lock->requested[i] >= 0 && lock->granted[i] >= 0);
                     if (lock->granted[i] == 0)
-                        lock->grantMask &= BITS_OFF(i);
+                        lock->grantMask &= LOCKBIT_OFF(i);

                     /*
                      * Read comments in LockRelease
diff -ruN ../base/src/backend/storage/lmgr/proc.c src/backend/storage/lmgr/proc.c
--- ../base/src/backend/storage/lmgr/proc.c    2003-09-06 22:43:50.000000000 +0200
+++ src/backend/storage/lmgr/proc.c    2003-09-07 07:35:06.000000000 +0200
@@ -531,7 +531,7 @@
 {
     LWLockId    masterLock = lockMethodTable->masterLock;
     PROC_QUEUE *waitQueue = &(lock->waitProcs);
-    int            myHeldLocks = MyProc->heldLocks;
+    LOCKMASK    myHeldLocks = MyProc->heldLocks;
     bool        early_deadlock = false;
     PGPROC       *proc;
     int            i;
@@ -556,7 +556,7 @@
      */
     if (myHeldLocks != 0)
     {
-        int            aheadRequests = 0;
+        LOCKMASK    aheadRequests = 0;

         proc = (PGPROC *) MAKE_PTR(waitQueue->links.next);
         for (i = 0; i < waitQueue->size; i++)
@@ -596,7 +596,7 @@
                 break;
             }
             /* Nope, so advance to next waiter */
-            aheadRequests |= (1 << proc->waitLockMode);
+            aheadRequests |= LOCKBIT_ON(proc->waitLockMode);
             proc = (PGPROC *) MAKE_PTR(proc->links.next);
         }

@@ -618,7 +618,7 @@
     SHMQueueInsertBefore(&(proc->links), &(MyProc->links));
     waitQueue->size++;

-    lock->waitMask |= (1 << lockmode);
+    lock->waitMask |= LOCKBIT_ON(lockmode);

     /* Set up wait information in PGPROC object, too */
     MyProc->waitLock = lock;
@@ -755,7 +755,7 @@
     PROC_QUEUE *waitQueue = &(lock->waitProcs);
     int            queue_size = waitQueue->size;
     PGPROC       *proc;
-    int            aheadRequests = 0;
+    LOCKMASK    aheadRequests = 0;

     Assert(queue_size >= 0);

@@ -796,7 +796,7 @@
              * Cannot wake this guy. Remember his request for later
              * checks.
              */
-            aheadRequests |= (1 << lockmode);
+            aheadRequests |= LOCKBIT_ON(lockmode);
             proc = (PGPROC *) MAKE_PTR(proc->links.next);
         }
     }
diff -ruN ../base/src/include/storage/lock.h src/include/storage/lock.h
--- ../base/src/include/storage/lock.h    2003-09-06 22:45:16.000000000 +0200
+++ src/include/storage/lock.h    2003-09-07 07:25:13.000000000 +0200
@@ -46,6 +46,9 @@
 /* MAX_LOCKMODES cannot be larger than the # of bits in LOCKMASK */
 #define MAX_LOCKMODES        10

+#define LOCKBIT_ON(lockmode) (1 << (lockmode))
+#define LOCKBIT_OFF(lockmode) (~(1 << (lockmode)))
+
 typedef uint16 LOCKMETHODID;
 /* MAX_LOCK_METHODS is the number of distinct lock control tables allowed */
 #define MAX_LOCK_METHODS    3