Re: Minor lmgr code cleanup

This has been saved for the 7.5 release:

http:/momjian.postgresql.org/cgi-bin/pgpatches2

---------------------------------------------------------------------------

Manfred Koizar wrote:
> On Sun, 07 Sep 2003 10:19:07 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073