Thread: Refactoring in lock.c

Patchers,

Here is a small patch to refactor common functionality out of
LockRelease and LockReleaseAll, creating a new static function
RemoveProcLock.

(This comes from Heikki's two-phase commit patch, where it is used more.)


Additionally, I found that no callers of LockReleaseAll and LockRelease
had any use for their return values, so I made them return void.

There are two exceptions to the "no use of return value" assertion: one
is the userlock contrib module, but I doubt it has much use for it in
reality;  I made it "return true" where it used LockRelease{,All} return
value, in order not change the function's signature; users have no way
to recover when user_unlock_all does not work anyway, so we are not
losing anything.

The other exception is LockReleaseCurrentOwner, where the only action
is to raise a WARNING.  Since LockRelease already raises WARNING for
anything that returns a FALSE value, we don't lose anything by just
returning void and dropping the warning in ResourceOwners.

Please apply if there are no objections.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

On Thu, May 12, 2005 at 11:06:59PM -0400, Alvaro Herrera wrote:

Hackers,

> Here is a small patch to refactor common functionality out of
> LockRelease and LockReleaseAll, creating a new static function
> RemoveProcLock.

Any word on this patch?  This is nearly trivial stuff.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"Un poeta es un mundo encerrado en un hombre" (Victor Hugo)

Alvaro Herrera wrote:
> Additionally, I found that no callers of LockReleaseAll and LockRelease
> had any use for their return values, so I made them return void.

Is there a reason we can't just elog(ERROR) rather than returning?

-Neil

On Wed, May 18, 2005 at 12:33:51PM +1000, Neil Conway wrote:
> Alvaro Herrera wrote:
> >Additionally, I found that no callers of LockReleaseAll and LockRelease
> >had any use for their return values, so I made them return void.
>
> Is there a reason we can't just elog(ERROR) rather than returning?

I thought about that too.  I'm not sure why the original code would
continue chugging along if the lock table is not consistent.  Maybe it
was because back in the Berkeley days this code would step on bugs with
some regularity.  I wonder if we could get away with changing it now.

Anyway, I didn't want to propose such a thing because a change in
functionality is not what I want to do in a refactor patch -- if the
idea is shot down, the whole thing is shot down and the patch is not
applied.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
Tulio: oh, para qué servirá este boton, Juan Carlos?
Policarpo: No, aléjense, no toquen la consola!
Juan Carlos: Lo apretaré una y otra vez.

Alvaro Herrera <alvherre@surnet.cl> writes:
> Here is a small patch to refactor common functionality out of
> LockRelease and LockReleaseAll, creating a new static function
> RemoveProcLock.
> (This comes from Heikki's two-phase commit patch, where it is used more.)

I was just looking at this code in the context of Heikki's patch, and
it seemed to have some issues: specifically that the code
        if (wakeupNeeded)
            ProcLockWakeup(lockMethodTable, lock);
was formerly run only if the lock still had nRequested > 0.  Since the
case where nRequested == 0 causes the lock to be physically removed,
it would not be merely inefficient but actually a use of a dangling
pointer to call ProcLockWakeup when the lock's been removed.  However
the patched code now does the above unconditionally.  Can you prove
that wakeupNeeded will never be true when nRequested == 0?

It might be safer to migrate the ProcLockWakeup call inside
RemoveProcLock.

FWIW, I agree with turning the WARNINGs into ERRORs and removing the
useless return value from LockRelease et al.

            regards, tom lane

Ok, new version of this patch, adjusted per your comments.  Thanks.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentre de él no son, por desgracia,
nada idílicas" (Ijon Tichy)

Alvaro Herrera <alvherre@surnet.cl> writes:
> Ok, new version of this patch, adjusted per your comments.  Thanks.

Applied with revisions.  I backed off the idea of removing LockRelease's
return value after looking at contrib/userlock; it seems possible that
some applications out there are depending on being able to issue
release for locks they don't hold.  Also fixed the logic: the code as
submitted was slightly wrong since it would fail to waken waiters if any
modes remain held.  ProcLockWakeup may need to be run even if we still
have a proclock.  So I renamed the function to CleanUpLock, which
hopefully is more indicative of what it really does.

Patch as applied is attached.

            regards, tom lane

*** contrib/userlock/user_locks.c.orig    Tue May 17 17:35:04 2005
--- contrib/userlock/user_locks.c    Thu May 19 18:31:26 2005
***************
*** 75,79 ****
  int
  user_unlock_all(void)
  {
!     return LockReleaseAll(USER_LOCKMETHOD, true);
  }
--- 75,81 ----
  int
  user_unlock_all(void)
  {
!     LockReleaseAll(USER_LOCKMETHOD, true);
!
!     return true;
  }
*** src/backend/storage/lmgr/lock.c.orig    Wed May 11 12:13:55 2005
--- src/backend/storage/lmgr/lock.c    Thu May 19 19:21:17 2005
***************
*** 166,171 ****
--- 166,173 ----
                   int *myHolding);
  static bool UnGrantLock(LOCK *lock, LOCKMODE lockmode,
                          PROCLOCK *proclock, LockMethod lockMethodTable);
+ static void CleanUpLock(LOCKMETHODID lockmethodid, LOCK *lock,
+                         PROCLOCK *proclock, bool wakeupNeeded);


  /*
***************
*** 589,601 ****
               * anyone to release the lock object later.
               */
              Assert(SHMQueueEmpty(&(lock->procLocks)));
!             lock = (LOCK *) hash_search(LockMethodLockHash[lockmethodid],
!                                         (void *) &(lock->tag),
!                                         HASH_REMOVE, NULL);
          }
          LWLockRelease(masterLock);
-         if (!lock)                /* hash remove failed? */
-             elog(WARNING, "lock table corrupted");
          ereport(ERROR,
                  (errcode(ERRCODE_OUT_OF_MEMORY),
                   errmsg("out of shared memory"),
--- 591,602 ----
               * anyone to release the lock object later.
               */
              Assert(SHMQueueEmpty(&(lock->procLocks)));
!             if (!hash_search(LockMethodLockHash[lockmethodid],
!                              (void *) &(lock->tag),
!                              HASH_REMOVE, NULL))
!                 elog(PANIC, "lock table corrupted");
          }
          LWLockRelease(masterLock);
          ereport(ERROR,
                  (errcode(ERRCODE_OUT_OF_MEMORY),
                   errmsg("out of shared memory"),
***************
*** 708,718 ****
              {
                  SHMQueueDelete(&proclock->lockLink);
                  SHMQueueDelete(&proclock->procLink);
!                 proclock = (PROCLOCK *) hash_search(LockMethodProcLockHash[lockmethodid],
!                                                (void *) &(proclock->tag),
!                                                     HASH_REMOVE, NULL);
!                 if (!proclock)
!                     elog(WARNING, "proclock table corrupted");
              }
              else
                  PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
--- 709,718 ----
              {
                  SHMQueueDelete(&proclock->lockLink);
                  SHMQueueDelete(&proclock->procLink);
!                 if (!hash_search(LockMethodProcLockHash[lockmethodid],
!                                  (void *) &(proclock->tag),
!                                  HASH_REMOVE, NULL))
!                     elog(PANIC, "proclock table corrupted");
              }
              else
                  PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
***************
*** 784,793 ****

      pfree(locallock->lockOwners);
      locallock->lockOwners = NULL;
!     locallock = (LOCALLOCK *) hash_search(LockMethodLocalHash[lockmethodid],
!                                           (void *) &(locallock->tag),
!                                           HASH_REMOVE, NULL);
!     if (!locallock)
          elog(WARNING, "locallock table corrupted");
  }

--- 784,792 ----

      pfree(locallock->lockOwners);
      locallock->lockOwners = NULL;
!     if (!hash_search(LockMethodLocalHash[lockmethodid],
!                      (void *) &(locallock->tag),
!                      HASH_REMOVE, NULL))
          elog(WARNING, "locallock table corrupted");
  }

***************
*** 994,999 ****
--- 993,1047 ----
  }

  /*
+  * CleanUpLock -- clean up after releasing a lock.  We garbage-collect the
+  * proclock and lock objects if possible, and call ProcLockWakeup if there
+  * are remaining requests and the caller says it's OK.  (Normally, this
+  * should be called after UnGrantLock, and wakeupNeeded is the result from
+  * UnGrantLock.)
+  *
+  * The locktable's masterLock must be held at entry, and will be
+  * held at exit.
+  */
+ static void
+ CleanUpLock(LOCKMETHODID lockmethodid, LOCK *lock, PROCLOCK *proclock,
+             bool wakeupNeeded)
+ {
+     /*
+      * If this was my last hold on this lock, delete my entry in the
+      * proclock table.
+      */
+     if (proclock->holdMask == 0)
+     {
+         PROCLOCK_PRINT("CleanUpLock: deleting", proclock);
+         SHMQueueDelete(&proclock->lockLink);
+         SHMQueueDelete(&proclock->procLink);
+         if (!hash_search(LockMethodProcLockHash[lockmethodid],
+                          (void *) &(proclock->tag),
+                          HASH_REMOVE, NULL))
+             elog(PANIC, "proclock table corrupted");
+     }
+
+     if (lock->nRequested == 0)
+     {
+         /*
+          * The caller just released the last lock, so garbage-collect the
+          * lock object.
+          */
+         LOCK_PRINT("CleanUpLock: deleting", lock, 0);
+         Assert(SHMQueueEmpty(&(lock->procLocks)));
+         if (!hash_search(LockMethodLockHash[lockmethodid],
+                          (void *) &(lock->tag),
+                          HASH_REMOVE, NULL))
+             elog(PANIC, "lock table corrupted");
+     }
+     else if (wakeupNeeded)
+     {
+         /* There are waiters on this lock, so wake them up. */
+         ProcLockWakeup(LockMethods[lockmethodid], lock);
+     }
+ }
+
+ /*
   * GrantLockLocal -- update the locallock data structures to show
   *        the lock request has been granted.
   *
***************
*** 1160,1189 ****

      /*
       * Delete the proclock immediately if it represents no already-held locks.
!      * This must happen now because if the owner of the lock decides to release
!      * it, and the requested/granted counts then go to zero, LockRelease
!      * expects there to be no remaining proclocks.
!      */
!     if (proclock->holdMask == 0)
!     {
!         PROCLOCK_PRINT("RemoveFromWaitQueue: deleting proclock", proclock);
!         SHMQueueDelete(&proclock->lockLink);
!         SHMQueueDelete(&proclock->procLink);
!         proclock = (PROCLOCK *) hash_search(LockMethodProcLockHash[lockmethodid],
!                                             (void *) &(proclock->tag),
!                                             HASH_REMOVE, NULL);
!         if (!proclock)
!             elog(WARNING, "proclock table corrupted");
!     }
!
!     /*
!      * There should still be some requests for the lock ... else what were
!      * we waiting for?  Therefore no need to delete the lock object.
       */
!     Assert(waitLock->nRequested > 0);
!
!     /* See if any other waiters for the lock can be woken up now */
!     ProcLockWakeup(LockMethods[lockmethodid], waitLock);
  }

  /*
--- 1208,1219 ----

      /*
       * Delete the proclock immediately if it represents no already-held locks.
!      * (This must happen now because if the owner of the lock decides to
!      * release it, and the requested/granted counts then go to zero,
!      * LockRelease expects there to be no remaining proclocks.)
!      * Then see if any other waiters for the lock can be woken up now.
       */
!     CleanUpLock(lockmethodid, waitLock, proclock, true);
  }

  /*
***************
*** 1221,1230 ****
      Assert(lockmethodid < NumLockMethods);
      lockMethodTable = LockMethods[lockmethodid];
      if (!lockMethodTable)
!     {
!         elog(WARNING, "lockMethodTable is null in LockRelease");
!         return FALSE;
!     }

      /*
       * Find the LOCALLOCK entry for this lock and lockmode
--- 1251,1257 ----
      Assert(lockmethodid < NumLockMethods);
      lockMethodTable = LockMethods[lockmethodid];
      if (!lockMethodTable)
!         elog(ERROR, "bad lock method: %d", lockmethodid);

      /*
       * Find the LOCALLOCK entry for this lock and lockmode
***************
*** 1328,1383 ****
          return FALSE;
      }

-     wakeupNeeded = UnGrantLock(lock, lockmode, proclock, lockMethodTable);
-
      /*
!      * If this was my last hold on this lock, delete my entry in the
!      * proclock table.
       */
!     if (proclock->holdMask == 0)
!     {
!         PROCLOCK_PRINT("LockRelease: deleting proclock", proclock);
!         SHMQueueDelete(&proclock->lockLink);
!         SHMQueueDelete(&proclock->procLink);
!         proclock = (PROCLOCK *) hash_search(LockMethodProcLockHash[lockmethodid],
!                                             (void *) &(proclock->tag),
!                                             HASH_REMOVE, NULL);
!         if (!proclock)
!         {
!             LWLockRelease(masterLock);
!             elog(WARNING, "proclock table corrupted");
!             RemoveLocalLock(locallock);
!             return FALSE;
!         }
!     }

!     if (lock->nRequested == 0)
!     {
!         /*
!          * We've just released the last lock, so garbage-collect the lock
!          * object.
!          */
!         LOCK_PRINT("LockRelease: deleting lock", lock, lockmode);
!         Assert(SHMQueueEmpty(&(lock->procLocks)));
!         lock = (LOCK *) hash_search(LockMethodLockHash[lockmethodid],
!                                     (void *) &(lock->tag),
!                                     HASH_REMOVE, NULL);
!         if (!lock)
!         {
!             LWLockRelease(masterLock);
!             elog(WARNING, "lock table corrupted");
!             RemoveLocalLock(locallock);
!             return FALSE;
!         }
!     }
!     else
!     {
!         /*
!          * Wake up waiters if needed.
!          */
!         if (wakeupNeeded)
!             ProcLockWakeup(lockMethodTable, lock);
!     }

      LWLockRelease(masterLock);

--- 1355,1366 ----
          return FALSE;
      }

      /*
!      * Do the releasing.  CleanUpLock will waken any now-wakable waiters.
       */
!     wakeupNeeded = UnGrantLock(lock, lockmode, proclock, lockMethodTable);

!     CleanUpLock(lockmethodid, lock, proclock, wakeupNeeded);

      LWLockRelease(masterLock);

***************
*** 1397,1403 ****
   * allxids == false: release all locks with Xid != 0
   * (zero is the Xid used for "session" locks).
   */
! bool
  LockReleaseAll(LOCKMETHODID lockmethodid, bool allxids)
  {
      HASH_SEQ_STATUS status;
--- 1380,1386 ----
   * allxids == false: release all locks with Xid != 0
   * (zero is the Xid used for "session" locks).
   */
! void
  LockReleaseAll(LOCKMETHODID lockmethodid, bool allxids)
  {
      HASH_SEQ_STATUS status;
***************
*** 1418,1427 ****
      Assert(lockmethodid < NumLockMethods);
      lockMethodTable = LockMethods[lockmethodid];
      if (!lockMethodTable)
!     {
!         elog(WARNING, "bad lock method: %d", lockmethodid);
!         return FALSE;
!     }

      numLockModes = lockMethodTable->numLockModes;
      masterLock = lockMethodTable->masterLock;
--- 1401,1407 ----
      Assert(lockmethodid < NumLockMethods);
      lockMethodTable = LockMethods[lockmethodid];
      if (!lockMethodTable)
!         elog(ERROR, "bad lock method: %d", lockmethodid);

      numLockModes = lockMethodTable->numLockModes;
      masterLock = lockMethodTable->masterLock;
***************
*** 1516,1563 ****
          Assert(lock->nGranted <= lock->nRequested);
          LOCK_PRINT("LockReleaseAll: updated", lock, 0);

!         PROCLOCK_PRINT("LockReleaseAll: deleting", proclock);
!
!         /*
!          * Remove the proclock entry from the linked lists
!          */
!         SHMQueueDelete(&proclock->lockLink);
!         SHMQueueDelete(&proclock->procLink);
!
!         /*
!          * remove the proclock entry from the hashtable
!          */
!         proclock = (PROCLOCK *) hash_search(LockMethodProcLockHash[lockmethodid],
!                                             (void *) &(proclock->tag),
!                                             HASH_REMOVE,
!                                             NULL);
!         if (!proclock)
!         {
!             LWLockRelease(masterLock);
!             elog(WARNING, "proclock table corrupted");
!             return FALSE;
!         }

!         if (lock->nRequested == 0)
!         {
!             /*
!              * We've just released the last lock, so garbage-collect the
!              * lock object.
!              */
!             LOCK_PRINT("LockReleaseAll: deleting", lock, 0);
!             Assert(SHMQueueEmpty(&(lock->procLocks)));
!             lock = (LOCK *) hash_search(LockMethodLockHash[lockmethodid],
!                                         (void *) &(lock->tag),
!                                         HASH_REMOVE, NULL);
!             if (!lock)
!             {
!                 LWLockRelease(masterLock);
!                 elog(WARNING, "lock table corrupted");
!                 return FALSE;
!             }
!         }
!         else if (wakeupNeeded)
!             ProcLockWakeup(lockMethodTable, lock);

  next_item:
          proclock = nextHolder;
--- 1496,1505 ----
          Assert(lock->nGranted <= lock->nRequested);
          LOCK_PRINT("LockReleaseAll: updated", lock, 0);

!         Assert(proclock->holdMask == 0);

!         /* CleanUpLock will wake up waiters if needed. */
!         CleanUpLock(lockmethodid, lock, proclock, wakeupNeeded);

  next_item:
          proclock = nextHolder;
***************
*** 1569,1576 ****
      if (lockmethodid == USER_LOCKMETHOD ? Trace_userlocks : Trace_locks)
          elog(LOG, "LockReleaseAll done");
  #endif
-
-     return TRUE;
  }

  /*
--- 1511,1516 ----
*** src/include/storage/lock.h.orig    Fri Apr 29 18:28:24 2005
--- src/include/storage/lock.h    Thu May 19 18:31:09 2005
***************
*** 361,367 ****
              TransactionId xid, LOCKMODE lockmode, bool dontWait);
  extern bool LockRelease(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
              TransactionId xid, LOCKMODE lockmode);
! extern bool LockReleaseAll(LOCKMETHODID lockmethodid, bool allxids);
  extern void LockReleaseCurrentOwner(void);
  extern void LockReassignCurrentOwner(void);
  extern int LockCheckConflicts(LockMethod lockMethodTable,
--- 361,367 ----
              TransactionId xid, LOCKMODE lockmode, bool dontWait);
  extern bool LockRelease(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
              TransactionId xid, LOCKMODE lockmode);
! extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allxids);
  extern void LockReleaseCurrentOwner(void);
  extern void LockReassignCurrentOwner(void);
  extern int LockCheckConflicts(LockMethod lockMethodTable,