Re: [PATCHES] Refactoring in lock.c
Alvaro Herrera <[EMAIL PROTECTED]> 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.origWed 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 *) &(locallo
Re: [PATCHES] Refactoring in lock.c
Ok, new version of this patch, adjusted per your comments. Thanks.
--
Alvaro Herrera ()
"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)
Index: contrib/userlock/user_locks.c
===
RCS file: /home/alvherre/cvs/pgsql/contrib/userlock/user_locks.c,v
retrieving revision 1.16
diff -c -r1.16 user_locks.c
*** contrib/userlock/user_locks.c 17 May 2005 21:46:09 - 1.16
--- contrib/userlock/user_locks.c 19 May 2005 01:30:55 -
***
*** 44,50
SET_LOCKTAG_USERLOCK(tag, id1, id2);
! return LockRelease(USER_LOCKMETHOD, &tag, InvalidTransactionId,
lockmode);
}
int
--- 44,52
SET_LOCKTAG_USERLOCK(tag, id1, id2);
! LockRelease(USER_LOCKMETHOD, &tag, InvalidTransactionId, lockmode);
!
! return true;
}
int
***
*** 75,79
int
user_unlock_all(void)
{
! return LockReleaseAll(USER_LOCKMETHOD, true);
}
--- 77,83
int
user_unlock_all(void)
{
! LockReleaseAll(USER_LOCKMETHOD, true);
!
! return true;
}
Index: src/backend/storage/lmgr/lock.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/storage/lmgr/lock.c,v
retrieving revision 1.151
diff -c -r1.151 lock.c
*** src/backend/storage/lmgr/lock.c 11 May 2005 01:26:02 - 1.151
--- src/backend/storage/lmgr/lock.c 19 May 2005 01:22:46 -
***
*** 166,171
--- 166,173
int *myHolding);
static bool UnGrantLock(LOCK *lock, LOCKMODE lockmode,
PROCLOCK *proclock, LockMethod
lockMethodTable);
+ static void RemoveProcLock(LOCKMETHODID lockmethodid, PROCLOCK *proclock,
+ LOCK *lock, bool wakeupNeeded);
/*
***
*** 792,797
--- 794,840
}
/*
+ * Subroutine to unlink and free a proclock entry, and garbage
+ * collect the lock object.
+ *
+ * The locktable's masterLock must be held at entry, and will be
+ * held at exit.
+ */
+ static void
+ RemoveProcLock(LOCKMETHODID lockmethodid, PROCLOCK *proclock, LOCK *lock,
+ bool wakeupNeeded)
+ {
+ PROCLOCK_PRINT("RemoveProcLock: deleting", proclock);
+ SHMQueueDelete(&proclock->lockLink);
+ SHMQueueDelete(&proclock->procLink);
+ proclock = (PROCLOCK *)
hash_search(LockMethodProcLockHash[lockmethodid],
+
(void *) &(proclock->tag),
+
HASH_REMOVE, NULL);
+ if (!proclock)
+ elog(ERROR, "proclock table corrupted");
+
+ if (lock->nRequested == 0)
+ {
+ /*
+* The caller just released the last lock, so garbage-collect
the
+* lock object.
+*/
+ LOCK_PRINT("RemoveProcLock: deleting", lock, 0);
+ Assert(SHMQueueEmpty(&(lock->procLocks)));
+ lock = (LOCK *) hash_search(LockMethodLockHash[lockmethodid],
+ (void
*) &(lock->tag),
+
HASH_REMOVE, NULL);
+ if (!lock)
+ elog(ERROR, "lock table corrupted");
+ }
+ else if (wakeupNeeded)
+ {
+ /* There are waiters on this lock, so wake them up. */
+ ProcLockWakeup(LockMethods[lockmethodid], lock);
+ }
+ }
+
+ /*
* LockCheckConflicts -- test whether requested lock conflicts
*with those already granted
*
***
*** 1196,1202
*the waking process and any new process to
*come along and request the lock.)
*/
! bool
LockRelease(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
TransactionId xid, LOCKMODE lockmode)
{
--- 1239,1245
*the waking process and any new process to
*come along and request the lock.)
*/
! void
LockRelease(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
TransactionId xid, LOCKMODE lockmode)
{
***
*** 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
--- 1264,1270
Assert(lockmethodid < NumLockMethods);
lockMethodTable = LockMethods[lockmethodid];
if (!lockMethodTable)
! e
Re: [PATCHES] Refactoring in lock.c
Alvaro Herrera <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] Refactoring in lock.c
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 () 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. ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Refactoring in lock.c
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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Refactoring in lock.c
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 () "Un poeta es un mundo encerrado en un hombre" (Victor Hugo) ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
[PATCHES] 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 ()
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)
Index: src/include/storage/lock.h
===
RCS file: /home/alvherre/cvs/pgsql/src/include/storage/lock.h,v
retrieving revision 1.85
diff -c -r1.85 lock.h
*** src/include/storage/lock.h 29 Apr 2005 22:28:24 - 1.85
--- src/include/storage/lock.h 13 May 2005 01:05:43 -
***
*** 359,367
extern LOCKMETHODID LockMethodTableRename(LOCKMETHODID lockmethodid);
extern bool LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
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,
--- 359,367
extern LOCKMETHODID LockMethodTableRename(LOCKMETHODID lockmethodid);
extern bool LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
TransactionId xid, LOCKMODE lockmode, bool dontWait);
! extern void 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,
Index: src/backend/storage/lmgr/lock.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/storage/lmgr/lock.c,v
retrieving revision 1.151
diff -c -r1.151 lock.c
*** src/backend/storage/lmgr/lock.c 11 May 2005 01:26:02 - 1.151
--- src/backend/storage/lmgr/lock.c 13 May 2005 01:05:23 -
***
*** 166,171
--- 166,172
int *myHolding);
static bool UnGrantLock(LOCK *lock, LOCKMODE lockmode,
PROCLOCK *proclock, LockMethod
lockMethodTable);
+ static bool RemoveProcLock(LOCKMETHODID lockmethodid, PROCLOCK *proclock,
LOCK *lock);
/*
***
*** 792,797
--- 793,840
}
/*
+ * Subroutine to unlink and free a proclock entry, and garbage
+ * collect the lock object.
+ *
+ * The locktable's masterLock must be held at entry, and will be
+ * held at exit.
+ */
+ static bool
+ RemoveProcLock(LOCKMETHODID lockmethodid, PROCLOCK *proclock, LOCK *lock)
+ {
+ PROCLOCK_PRINT("RemoveProcLock: deleting", 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");
+ return FALSE;
+ }
+
+ if (lock->nRequested == 0)
+ {
+ /*
+* We've just released the last lock, so garbage-collect the
+* lock object.
+*/
+ LOCK_PRINT("RemoveProcLock: deleting", lock, 0);
+ Assert(SHMQueueEmpty(&(lock->procLocks)));
+ lock = (LOCK *) hash_search(LockMethodLockHash[lockmethodid],
+ (void
*) &(lock->tag),
+
HASH_REMOVE, NULL);
+ if (!lock)
+ {
+
