Skip to content

Commit 140b078

Browse files
tglsfdctglsfdc
committed
Improve LockAcquire API per my recent proposal. All error conditions
are now reported via elog, eliminating the need to test the result code at most call sites. Make it possible for the caller to distinguish a freshly acquired lock from one already held in the current transaction. Use that capability to avoid redundant AcceptInvalidationMessages() calls in LockRelation().
1 parent 299c442 commit 140b078

File tree

4 files changed

+81
-69
lines changed

4 files changed

+81
-69
lines changed

‎contrib/userlock/user_locks.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ user_lock(uint32 id1, uint32 id2, LOCKMODE lockmode)
3333

3434
SET_LOCKTAG_USERLOCK(tag, id1, id2);
3535

36-
return LockAcquire(USER_LOCKMETHOD, &tag, InvalidTransactionId,
37-
lockmode, true);
36+
return (LockAcquire(USER_LOCKMETHOD, &tag, InvalidTransactionId,
37+
lockmode, true) != LOCKACQUIRE_NOT_AVAIL);
3838
}
3939

4040
int

‎src/backend/storage/lmgr/lmgr.c

Lines changed: 44 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/storage/lmgr/lmgr.c,v 1.74 2005/05/19 21:35:46 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/storage/lmgr/lmgr.c,v 1.75 2005/05/29 22:45:02 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -136,24 +136,28 @@ void
136136
LockRelation(Relation relation, LOCKMODE lockmode)
137137
{
138138
LOCKTAG tag;
139+
LockAcquireResult res;
139140

140141
SET_LOCKTAG_RELATION(tag,
141142
relation->rd_lockInfo.lockRelId.dbId,
142143
relation->rd_lockInfo.lockRelId.relId);
143144

144-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
145-
lockmode, false))
146-
elog(ERROR, "LockAcquire failed");
145+
res = LockAcquire(LockTableId, &tag, GetTopTransactionId(),
146+
lockmode, false);
147147

148148
/*
149149
* Check to see if the relcache entry has been invalidated while we
150150
* were waiting to lock it. If so, rebuild it, or ereport() trying.
151151
* Increment the refcount to ensure that RelationFlushRelation will
152-
* rebuild it and not just delete it.
152+
* rebuild it and not just delete it. We can skip this if the lock
153+
* was already held, however.
153154
*/
154-
RelationIncrementReferenceCount(relation);
155-
AcceptInvalidationMessages();
156-
RelationDecrementReferenceCount(relation);
155+
if (res != LOCKACQUIRE_ALREADY_HELD)
156+
{
157+
RelationIncrementReferenceCount(relation);
158+
AcceptInvalidationMessages();
159+
RelationDecrementReferenceCount(relation);
160+
}
157161
}
158162

159163
/*
@@ -169,24 +173,31 @@ bool
169173
ConditionalLockRelation(Relation relation, LOCKMODE lockmode)
170174
{
171175
LOCKTAG tag;
176+
LockAcquireResult res;
172177

173178
SET_LOCKTAG_RELATION(tag,
174179
relation->rd_lockInfo.lockRelId.dbId,
175180
relation->rd_lockInfo.lockRelId.relId);
176181

177-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
178-
lockmode, true))
182+
res = LockAcquire(LockTableId, &tag, GetTopTransactionId(),
183+
lockmode, true);
184+
185+
if (res == LOCKACQUIRE_NOT_AVAIL)
179186
return false;
180187

181188
/*
182189
* Check to see if the relcache entry has been invalidated while we
183190
* were waiting to lock it. If so, rebuild it, or ereport() trying.
184191
* Increment the refcount to ensure that RelationFlushRelation will
185-
* rebuild it and not just delete it.
192+
* rebuild it and not just delete it. We can skip this if the lock
193+
* was already held, however.
186194
*/
187-
RelationIncrementReferenceCount(relation);
188-
AcceptInvalidationMessages();
189-
RelationDecrementReferenceCount(relation);
195+
if (res != LOCKACQUIRE_ALREADY_HELD)
196+
{
197+
RelationIncrementReferenceCount(relation);
198+
AcceptInvalidationMessages();
199+
RelationDecrementReferenceCount(relation);
200+
}
190201

191202
return true;
192203
}
@@ -225,9 +236,8 @@ LockRelationForSession(LockRelId *relid, LOCKMODE lockmode)
225236

226237
SET_LOCKTAG_RELATION(tag, relid->dbId, relid->relId);
227238

228-
if (!LockAcquire(LockTableId, &tag, InvalidTransactionId,
229-
lockmode, false))
230-
elog(ERROR, "LockAcquire failed");
239+
(void) LockAcquire(LockTableId, &tag, InvalidTransactionId,
240+
lockmode, false);
231241
}
232242

233243
/*
@@ -262,9 +272,8 @@ LockRelationForExtension(Relation relation, LOCKMODE lockmode)
262272
relation->rd_lockInfo.lockRelId.dbId,
263273
relation->rd_lockInfo.lockRelId.relId);
264274

265-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
266-
lockmode, false))
267-
elog(ERROR, "LockAcquire failed");
275+
(void) LockAcquire(LockTableId, &tag, GetTopTransactionId(),
276+
lockmode, false);
268277
}
269278

270279
/*
@@ -298,9 +307,8 @@ LockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode)
298307
relation->rd_lockInfo.lockRelId.relId,
299308
blkno);
300309

301-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
302-
lockmode, false))
303-
elog(ERROR, "LockAcquire failed");
310+
(void) LockAcquire(LockTableId, &tag, GetTopTransactionId(),
311+
lockmode, false);
304312
}
305313

306314
/*
@@ -319,8 +327,8 @@ ConditionalLockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode)
319327
relation->rd_lockInfo.lockRelId.relId,
320328
blkno);
321329

322-
return LockAcquire(LockTableId, &tag, GetTopTransactionId(),
323-
lockmode, true);
330+
return (LockAcquire(LockTableId, &tag, GetTopTransactionId(),
331+
lockmode, true) != LOCKACQUIRE_NOT_AVAIL);
324332
}
325333

326334
/*
@@ -357,9 +365,8 @@ LockTuple(Relation relation, ItemPointer tid, LOCKMODE lockmode)
357365
ItemPointerGetBlockNumber(tid),
358366
ItemPointerGetOffsetNumber(tid));
359367

360-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
361-
lockmode, false))
362-
elog(ERROR, "LockAcquire failed");
368+
(void) LockAcquire(LockTableId, &tag, GetTopTransactionId(),
369+
lockmode, false);
363370
}
364371

365372
/*
@@ -393,9 +400,8 @@ XactLockTableInsert(TransactionId xid)
393400

394401
SET_LOCKTAG_TRANSACTION(tag, xid);
395402

396-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
397-
ExclusiveLock, false))
398-
elog(ERROR, "LockAcquire failed");
403+
(void) LockAcquire(LockTableId, &tag, GetTopTransactionId(),
404+
ExclusiveLock, false);
399405
}
400406

401407
/*
@@ -441,8 +447,9 @@ XactLockTableWait(TransactionId xid)
441447

442448
SET_LOCKTAG_TRANSACTION(tag, xid);
443449

444-
if (!LockAcquire(LockTableId, &tag, myxid, ShareLock, false))
445-
elog(ERROR, "LockAcquire failed");
450+
(void) LockAcquire(LockTableId, &tag, myxid,
451+
ShareLock, false);
452+
446453
LockRelease(LockTableId, &tag, myxid, ShareLock);
447454

448455
if (!TransactionIdIsInProgress(xid))
@@ -479,9 +486,8 @@ LockDatabaseObject(Oid classid, Oid objid, uint16 objsubid,
479486
objid,
480487
objsubid);
481488

482-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
483-
lockmode, false))
484-
elog(ERROR, "LockAcquire failed");
489+
(void) LockAcquire(LockTableId, &tag, GetTopTransactionId(),
490+
lockmode, false);
485491
}
486492

487493
/*
@@ -519,9 +525,8 @@ LockSharedObject(Oid classid, Oid objid, uint16 objsubid,
519525
objid,
520526
objsubid);
521527

522-
if (!LockAcquire(LockTableId, &tag, GetTopTransactionId(),
523-
lockmode, false))
524-
elog(ERROR, "LockAcquire failed");
528+
(void) LockAcquire(LockTableId, &tag, GetTopTransactionId(),
529+
lockmode, false);
525530
}
526531

527532
/*

‎src/backend/storage/lmgr/lock.c

Lines changed: 24 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/storage/lmgr/lock.c,v 1.153 2005/05/29 04:23:04 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/storage/lmgr/lock.c,v 1.154 2005/05/29 22:45:02 tgl Exp $
1212
*
1313
* NOTES
1414
* Outside modules can create a lock table and acquire/release
@@ -160,8 +160,8 @@ PROCLOCK_PRINT(const char *where, const PROCLOCK *proclockP)
160160

161161
static void RemoveLocalLock(LOCALLOCK *locallock);
162162
static void GrantLockLocal(LOCALLOCK *locallock, ResourceOwner owner);
163-
static int WaitOnLock(LOCKMETHODID lockmethodid, LOCALLOCK *locallock,
164-
ResourceOwner owner);
163+
static void WaitOnLock(LOCKMETHODID lockmethodid, LOCALLOCK *locallock,
164+
ResourceOwner owner);
165165
static void LockCountMyLocks(SHMEM_OFFSET lockOffset, PGPROC *proc,
166166
int *myHolding);
167167
static bool UnGrantLock(LOCK *lock, LOCKMODE lockmode,
@@ -377,11 +377,14 @@ LockMethodTableRename(LOCKMETHODID lockmethodid)
377377
* LockAcquire -- Check for lock conflicts, sleep if conflict found,
378378
* set lock if/when no conflicts.
379379
*
380-
* Returns: TRUE if lock was acquired, FALSE otherwise. Note that
381-
* a FALSE return is to be expected if dontWait is TRUE;
382-
* but if dontWait is FALSE, only a parameter error can cause
383-
* a FALSE return. (XXX probably we should just ereport on parameter
384-
* errors, instead of conflating this with failure to acquire lock?)
380+
* Returns one of:
381+
* LOCKACQUIRE_NOT_AVAIL lock not available, and dontWait=true
382+
* LOCKACQUIRE_OK lock successfully acquired
383+
* LOCKACQUIRE_ALREADY_HELD incremented count for lock already held
384+
*
385+
* In the normal case where dontWait=false and the caller doesn't need to
386+
* distinguish a freshly acquired lock from one already taken earlier in
387+
* this same transaction, there is no need to examine the return value.
385388
*
386389
* Side Effects: The lock is acquired and recorded in lock tables.
387390
*
@@ -416,8 +419,7 @@ LockMethodTableRename(LOCKMETHODID lockmethodid)
416419
*
417420
* DZ - 22 Nov 1997
418421
*/
419-
420-
bool
422+
LockAcquireResult
421423
LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
422424
TransactionId xid, LOCKMODE lockmode, bool dontWait)
423425
{
@@ -447,10 +449,7 @@ LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
447449
Assert(lockmethodid < NumLockMethods);
448450
lockMethodTable = LockMethods[lockmethodid];
449451
if (!lockMethodTable)
450-
{
451-
elog(WARNING, "bad lock table id: %d", lockmethodid);
452-
return FALSE;
453-
}
452+
elog(ERROR, "unrecognized lock method: %d", lockmethodid);
454453

455454
/* Session locks and user locks are not transactional */
456455
if (xid != InvalidTransactionId &&
@@ -507,7 +506,7 @@ LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
507506
if (locallock->nLocks > 0)
508507
{
509508
GrantLockLocal(locallock, owner);
510-
return TRUE;
509+
return LOCKACQUIRE_ALREADY_HELD;
511510
}
512511

513512
/*
@@ -669,7 +668,7 @@ LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
669668
GrantLockLocal(locallock, owner);
670669
PROCLOCK_PRINT("LockAcquire: my other XID owning", proclock);
671670
LWLockRelease(masterLock);
672-
return TRUE;
671+
return LOCKACQUIRE_ALREADY_HELD;
673672
}
674673

675674
/*
@@ -696,8 +695,8 @@ LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
696695

697696
/*
698697
* We can't acquire the lock immediately. If caller specified no
699-
* blocking, remove useless table entries and return FALSE without
700-
* waiting.
698+
* blocking, remove useless table entries and return NOT_AVAIL
699+
* without waiting.
701700
*/
702701
if (dontWait)
703702
{
@@ -720,7 +719,7 @@ LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
720719
LWLockRelease(masterLock);
721720
if (locallock->nLocks == 0)
722721
RemoveLocalLock(locallock);
723-
return FALSE;
722+
return LOCKACQUIRE_NOT_AVAIL;
724723
}
725724

726725
/*
@@ -740,7 +739,7 @@ LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
740739
/*
741740
* Sleep till someone wakes me up.
742741
*/
743-
status = WaitOnLock(lockmethodid, locallock, owner);
742+
WaitOnLock(lockmethodid, locallock, owner);
744743

745744
/*
746745
* NOTE: do not do any material change of state between here and
@@ -759,15 +758,15 @@ LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
759758
LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode);
760759
/* Should we retry ? */
761760
LWLockRelease(masterLock);
762-
return FALSE;
761+
elog(ERROR, "LockAcquire failed");
763762
}
764763
PROCLOCK_PRINT("LockAcquire: granted", proclock);
765764
LOCK_PRINT("LockAcquire: granted", lock, lockmode);
766765
}
767766

768767
LWLockRelease(masterLock);
769768

770-
return status == STATUS_OK;
769+
return LOCKACQUIRE_OK;
771770
}
772771

773772
/*
@@ -1091,7 +1090,7 @@ GrantAwaitedLock(void)
10911090
*
10921091
* The locktable's masterLock must be held at entry.
10931092
*/
1094-
static int
1093+
static void
10951094
WaitOnLock(LOCKMETHODID lockmethodid, LOCALLOCK *locallock,
10961095
ResourceOwner owner)
10971096
{
@@ -1159,7 +1158,6 @@ WaitOnLock(LOCKMETHODID lockmethodid, LOCALLOCK *locallock,
11591158

11601159
LOCK_PRINT("WaitOnLock: wakeup on lock",
11611160
locallock->lock, locallock->tag.mode);
1162-
return STATUS_OK;
11631161
}
11641162

11651163
/*
@@ -1247,7 +1245,7 @@ LockRelease(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
12471245
Assert(lockmethodid < NumLockMethods);
12481246
lockMethodTable = LockMethods[lockmethodid];
12491247
if (!lockMethodTable)
1250-
elog(ERROR, "bad lock method: %d", lockmethodid);
1248+
elog(ERROR, "unrecognized lock method: %d", lockmethodid);
12511249

12521250
/*
12531251
* Find the LOCALLOCK entry for this lock and lockmode
@@ -1397,7 +1395,7 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allxids)
13971395
Assert(lockmethodid < NumLockMethods);
13981396
lockMethodTable = LockMethods[lockmethodid];
13991397
if (!lockMethodTable)
1400-
elog(ERROR, "bad lock method: %d", lockmethodid);
1398+
elog(ERROR, "unrecognized lock method: %d", lockmethodid);
14011399

14021400
numLockModes = lockMethodTable->numLockModes;
14031401
masterLock = lockMethodTable->masterLock;

‎src/include/storage/lock.h

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/storage/lock.h,v 1.86 2005/05/19 23:30:18 tgl Exp $
10+
* $PostgreSQL: pgsql/src/include/storage/lock.h,v 1.87 2005/05/29 22:45:02 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -348,6 +348,15 @@ typedef struct
348348
} LockData;
349349

350350

351+
/* Result codes for LockAcquire() */
352+
typedef enum
353+
{
354+
LOCKACQUIRE_NOT_AVAIL, /* lock not available, and dontWait=true */
355+
LOCKACQUIRE_OK, /* lock successfully acquired */
356+
LOCKACQUIRE_ALREADY_HELD /* incremented count for lock already held */
357+
} LockAcquireResult;
358+
359+
351360
/*
352361
* function prototypes
353362
*/
@@ -357,7 +366,7 @@ extern LOCKMETHODID LockMethodTableInit(const char *tabName,
357366
const LOCKMASK *conflictsP,
358367
int numModes, int maxBackends);
359368
extern LOCKMETHODID LockMethodTableRename(LOCKMETHODID lockmethodid);
360-
extern bool LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
369+
extern LockAcquireResult LockAcquire(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
361370
TransactionId xid, LOCKMODE lockmode, bool dontWait);
362371
extern bool LockRelease(LOCKMETHODID lockmethodid, LOCKTAG *locktag,
363372
TransactionId xid, LOCKMODE lockmode);

0 commit comments

Comments
 (0)