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

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@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

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

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