From 1abaf5c17ddb92a14bd20afc0e43ad3fc21a8475 Mon Sep 17 00:00:00 2001 From: Matthias van de Meent Date: Wed, 20 Nov 2024 03:27:32 +0100 Subject: [PATCH v0 1/4] Reduce size of LOCK by 16 bytes We only ever need 8 lockmodes, rather than 10, as NoLock is never registered, and MaxLockmode is AccessExclusiveLock (8). Also adjust various locations where we assume MAX_LOCKMODES > MaxLockMode. --- src/include/storage/lock.h | 4 +- src/backend/access/common/relation.c | 6 +-- src/backend/access/index/indexam.c | 2 +- src/backend/storage/lmgr/README | 15 +++--- src/backend/storage/lmgr/lock.c | 76 ++++++++++++++++------------ src/backend/utils/adt/lockfuncs.c | 2 +- 6 files changed, 59 insertions(+), 46 deletions(-) diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h index 787f3db06a..b2523bf79d 100644 --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -79,11 +79,13 @@ typedef struct (vxid_dst).localTransactionId = (proc).vxid.lxid) /* MAX_LOCKMODES cannot be larger than the # of bits in LOCKMASK */ -#define MAX_LOCKMODES 10 +#define MAX_LOCKMODES MaxLockMode #define LOCKBIT_ON(lockmode) (1 << (lockmode)) #define LOCKBIT_OFF(lockmode) (~(1 << (lockmode))) +#define LOCK_VALID_LOCKMODE(lockmode) \ + ((lockmode) > NoLock && (lockmode) <= MaxLockMode) /* * This data structure defines the locking semantics associated with a diff --git a/src/backend/access/common/relation.c b/src/backend/access/common/relation.c index d8a313a2c9..78e58d13ce 100644 --- a/src/backend/access/common/relation.c +++ b/src/backend/access/common/relation.c @@ -48,7 +48,7 @@ relation_open(Oid relationId, LOCKMODE lockmode) { Relation r; - Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES); + Assert(lockmode >= NoLock && lockmode <= MAX_LOCKMODES); /* Get the lock before trying to open the relcache entry */ if (lockmode != NoLock) @@ -89,7 +89,7 @@ try_relation_open(Oid relationId, LOCKMODE lockmode) { Relation r; - Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES); + Assert(lockmode >= NoLock && lockmode <= MAX_LOCKMODES); /* Get the lock first */ if (lockmode != NoLock) @@ -206,7 +206,7 @@ relation_close(Relation relation, LOCKMODE lockmode) { LockRelId relid = relation->rd_lockInfo.lockRelId; - Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES); + Assert(lockmode >= NoLock && lockmode <= MAX_LOCKMODES); /* The relcache does the real work... */ RelationClose(relation); diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c index 1859be614c..70b9ac120e 100644 --- a/src/backend/access/index/indexam.c +++ b/src/backend/access/index/indexam.c @@ -178,7 +178,7 @@ index_close(Relation relation, LOCKMODE lockmode) { LockRelId relid = relation->rd_lockInfo.lockRelId; - Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES); + Assert(lockmode >= NoLock && lockmode <= MAX_LOCKMODES); /* The relcache does the real work... */ RelationClose(relation); diff --git a/src/backend/storage/lmgr/README b/src/backend/storage/lmgr/README index 45de0fd2bd..f85a0d6020 100644 --- a/src/backend/storage/lmgr/README +++ b/src/backend/storage/lmgr/README @@ -105,7 +105,7 @@ grantMask - table) to determine if a new lock request will conflict with existing lock types held. Conflicts are determined by bitwise AND operations between the grantMask and the conflict table entry for the requested - lock type. Bit i of grantMask is 1 if and only if granted[i] > 0. + lock type. Bit i of grantMask is 1 if and only if granted[i - 1] > 0. waitMask - This bitmask shows the types of locks being waited for. Bit i of waitMask @@ -133,10 +133,10 @@ nRequested - only in the backend's LOCALLOCK structure.) requested - - Keeps a count of how many locks of each type have been attempted. Only - elements 1 through MAX_LOCKMODES-1 are used as they correspond to the lock - type defined constants. Summing the values of requested[] should come out - equal to nRequested. + Keeps a count of how many locks of each type have been attempted. + Elements 0 through MAX_LOCKMODES (inclusive) are used, and correspond to + lock type definded constants with value elem+1. Summing the values of + requested[] should come out equal to nRequested. nGranted - Keeps count of how many times this lock has been successfully acquired. @@ -145,9 +145,8 @@ nGranted - granted - Keeps count of how many locks of each type are currently held. Once again - only elements 1 through MAX_LOCKMODES-1 are used (0 is not). Also, like - requested[], summing the values of granted[] should total to the value - of nGranted. + only elements 0 through MAX_LOCKMODES are used. Also, like requested[], + summing the values of granted[] should total to the value of nGranted. We should always have 0 <= nGranted <= nRequested, and 0 <= granted[i] <= requested[i] for each i. When all the request counts diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index edc5020c6a..9bf6fbf976 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -367,19 +367,19 @@ LOCK_PRINT(const char *where, const LOCK *lock, LOCKMODE type) if (LOCK_DEBUG_ENABLED(&lock->tag)) elog(LOG, "%s: lock(%p) id(%u,%u,%u,%u,%u,%u) grantMask(%x) " - "req(%d,%d,%d,%d,%d,%d,%d)=%d " - "grant(%d,%d,%d,%d,%d,%d,%d)=%d wait(%d) type(%s)", + "req(%d,%d,%d,%d,%d,%d,%d,%d)=%d " + "grant(%d,%d,%d,%d,%d,%d,%d,%d)=%d wait(%d) type(%s)", where, lock, lock->tag.locktag_field1, lock->tag.locktag_field2, lock->tag.locktag_field3, lock->tag.locktag_field4, lock->tag.locktag_type, lock->tag.locktag_lockmethodid, lock->grantMask, - lock->requested[1], lock->requested[2], lock->requested[3], - lock->requested[4], lock->requested[5], lock->requested[6], - lock->requested[7], lock->nRequested, - lock->granted[1], lock->granted[2], lock->granted[3], - lock->granted[4], lock->granted[5], lock->granted[6], - lock->granted[7], lock->nGranted, + lock->requested[0], lock->requested[1], lock->requested[2], + lock->requested[3], lock->requested[4], lock->requested[5], + lock->requested[6], lock->requested[7], lock->nRequested, + lock->granted[0], lock->granted[1], lock->granted[2], + lock->granted[3], lock->granted[4], lock->granted[5], + lock->granted[6], lock->granted[7], lock->nGranted, dclist_count(&lock->waitProcs), LockMethods[LOCK_LOCKMETHOD(*lock)]->lockModeNames[type]); } @@ -704,6 +704,8 @@ LockHasWaiters(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock) if (lockmode <= 0 || lockmode > lockMethodTable->numLockModes) elog(ERROR, "unrecognized lock mode: %d", lockmode); + Assert(LOCK_VALID_LOCKMODE(lockmode)); + #ifdef LOCK_DEBUG if (LOCK_DEBUG_ENABLED(locktag)) elog(LOG, "LockHasWaiters: lock [%u,%u] %s", @@ -851,6 +853,8 @@ LockAcquireExtended(const LOCKTAG *locktag, if (lockmode <= 0 || lockmode > lockMethodTable->numLockModes) elog(ERROR, "unrecognized lock mode: %d", lockmode); + Assert(LOCK_VALID_LOCKMODE(lockmode)); + if (RecoveryInProgress() && !InRecovery && (locktag->locktag_type == LOCKTAG_OBJECT || locktag->locktag_type == LOCKTAG_RELATION) && @@ -1133,11 +1137,11 @@ LockAcquireExtended(const LOCKTAG *locktag, else PROCLOCK_PRINT("LockAcquire: did not join wait queue", proclock); lock->nRequested--; - lock->requested[lockmode]--; + lock->requested[lockmode - 1]--; LOCK_PRINT("LockAcquire: did not join wait queue", lock, lockmode); Assert((lock->nRequested > 0) && - (lock->requested[lockmode] >= 0)); + (lock->requested[lockmode - 1] >= 0)); Assert(lock->nGranted <= lock->nRequested); LWLockRelease(partitionLock); if (locallock->nLocks == 0) @@ -1237,6 +1241,7 @@ SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc, PROCLOCKTAG proclocktag; uint32 proclock_hashcode; bool found; + Assert(LOCK_VALID_LOCKMODE(lockmode)); /* * Find or create a lock with this tag. @@ -1267,8 +1272,8 @@ SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc, else { LOCK_PRINT("LockAcquire: found", lock, lockmode); - Assert((lock->nRequested >= 0) && (lock->requested[lockmode] >= 0)); - Assert((lock->nGranted >= 0) && (lock->granted[lockmode] >= 0)); + Assert((lock->nRequested >= 0) && (lock->requested[lockmode - 1] >= 0)); + Assert((lock->nGranted >= 0) && (lock->granted[lockmode - 1] >= 0)); Assert(lock->nGranted <= lock->nRequested); } @@ -1385,8 +1390,8 @@ SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc, * The other counts don't increment till we get the lock. */ lock->nRequested++; - lock->requested[lockmode]++; - Assert((lock->nRequested > 0) && (lock->requested[lockmode] > 0)); + lock->requested[lockmode - 1]++; + Assert((lock->nRequested > 0) && (lock->requested[lockmode - 1] > 0)); /* * We shouldn't already hold the desired lock; else locallock table is @@ -1483,7 +1488,7 @@ LockCheckConflicts(LockMethod lockMethodTable, int numLockModes = lockMethodTable->numLockModes; LOCKMASK myLocks; int conflictMask = lockMethodTable->conflictTab[lockmode]; - int conflictsRemaining[MAX_LOCKMODES]; + int conflictsRemaining[MAX_LOCKMODES + 1]; int totalConflictsRemaining = 0; dlist_iter proclock_iter; int i; @@ -1516,7 +1521,7 @@ LockCheckConflicts(LockMethod lockMethodTable, conflictsRemaining[i] = 0; continue; } - conflictsRemaining[i] = lock->granted[i]; + conflictsRemaining[i] = lock->granted[i - 1]; if (myLocks & LOCKBIT_ON(i)) --conflictsRemaining[i]; totalConflictsRemaining += conflictsRemaining[i]; @@ -1606,14 +1611,16 @@ LockCheckConflicts(LockMethod lockMethodTable, void GrantLock(LOCK *lock, PROCLOCK *proclock, LOCKMODE lockmode) { + Assert(LOCK_VALID_LOCKMODE(lockmode)); + lock->nGranted++; - lock->granted[lockmode]++; + lock->granted[lockmode - 1]++; lock->grantMask |= LOCKBIT_ON(lockmode); - if (lock->granted[lockmode] == lock->requested[lockmode]) + if (lock->granted[lockmode - 1] == lock->requested[lockmode - 1]) lock->waitMask &= LOCKBIT_OFF(lockmode); proclock->holdMask |= LOCKBIT_ON(lockmode); LOCK_PRINT("GrantLock", lock, lockmode); - Assert((lock->nGranted > 0) && (lock->granted[lockmode] > 0)); + Assert((lock->nGranted > 0) && (lock->granted[lockmode - 1] > 0)); Assert(lock->nGranted <= lock->nRequested); } @@ -1631,20 +1638,21 @@ UnGrantLock(LOCK *lock, LOCKMODE lockmode, PROCLOCK *proclock, LockMethod lockMethodTable) { bool wakeupNeeded = false; + Assert(LOCK_VALID_LOCKMODE(lockmode)); - Assert((lock->nRequested > 0) && (lock->requested[lockmode] > 0)); - Assert((lock->nGranted > 0) && (lock->granted[lockmode] > 0)); + Assert((lock->nRequested > 0) && (lock->requested[lockmode - 1] > 0)); + Assert((lock->nGranted > 0) && (lock->granted[lockmode - 1] > 0)); Assert(lock->nGranted <= lock->nRequested); /* * fix the general lock stats */ lock->nRequested--; - lock->requested[lockmode]--; + lock->requested[lockmode - 1]--; lock->nGranted--; - lock->granted[lockmode]--; + lock->granted[lockmode - 1]--; - if (lock->granted[lockmode] == 0) + if (lock->granted[lockmode - 1] == 0) { /* change the conflict mask. No more of this lock type. */ lock->grantMask &= LOCKBIT_OFF(lockmode); @@ -1656,7 +1664,7 @@ UnGrantLock(LOCK *lock, LOCKMODE lockmode, * We need only run ProcLockWakeup if the released lock conflicts with at * least one of the lock types requested by waiter(s). Otherwise whatever * conflict made them wait must still exist. NOTE: before MVCC, we could - * skip wakeup if lock->granted[lockmode] was still positive. But that's + * skip wakeup if lock->granted[lockmode - 1] was still positive. But that's * not true anymore, because the remaining granted locks might belong to * some waiter, who could now be awakened because he doesn't conflict with * his own locks. @@ -1973,10 +1981,10 @@ RemoveFromWaitQueue(PGPROC *proc, uint32 hashcode) Assert(waitLock->nRequested > 0); Assert(waitLock->nRequested > proc->waitLock->nGranted); waitLock->nRequested--; - Assert(waitLock->requested[lockmode] > 0); - waitLock->requested[lockmode]--; + Assert(waitLock->requested[lockmode - 1] > 0); + waitLock->requested[lockmode - 1]--; /* don't forget to clear waitMask bit if appropriate */ - if (waitLock->granted[lockmode] == waitLock->requested[lockmode]) + if (waitLock->granted[lockmode - 1] == waitLock->requested[lockmode - 1]) waitLock->waitMask &= LOCKBIT_OFF(lockmode); /* Clean up the proc's own state, and pass it the ok/fail signal */ @@ -2025,6 +2033,8 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock) if (lockmode <= 0 || lockmode > lockMethodTable->numLockModes) elog(ERROR, "unrecognized lock mode: %d", lockmode); + Assert(LOCK_VALID_LOCKMODE(lockmode)); + #ifdef LOCK_DEBUG if (LOCK_DEBUG_ENABLED(locktag)) elog(LOG, "LockRelease: lock [%u,%u] %s", @@ -4311,6 +4321,8 @@ lock_twophase_recover(TransactionId xid, uint16 info, errhint("You might need to increase \"%s\".", "max_locks_per_transaction"))); } + Assert(LOCK_VALID_LOCKMODE(lockmode)); + /* * if it's a new lock object, initialize it */ @@ -4329,8 +4341,8 @@ lock_twophase_recover(TransactionId xid, uint16 info, else { LOCK_PRINT("lock_twophase_recover: found", lock, lockmode); - Assert((lock->nRequested >= 0) && (lock->requested[lockmode] >= 0)); - Assert((lock->nGranted >= 0) && (lock->granted[lockmode] >= 0)); + Assert((lock->nRequested >= 0) && (lock->requested[lockmode - 1] >= 0)); + Assert((lock->nGranted >= 0) && (lock->granted[lockmode - 1] >= 0)); Assert(lock->nGranted <= lock->nRequested); } @@ -4402,8 +4414,8 @@ lock_twophase_recover(TransactionId xid, uint16 info, * requests, whether granted or waiting, so increment those immediately. */ lock->nRequested++; - lock->requested[lockmode]++; - Assert((lock->nRequested > 0) && (lock->requested[lockmode] > 0)); + lock->requested[lockmode - 1]++; + Assert((lock->nRequested > 0) && (lock->requested[lockmode - 1] > 0)); /* * We shouldn't already hold the desired lock. diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c index e790f856ab..f21f348464 100644 --- a/src/backend/utils/adt/lockfuncs.c +++ b/src/backend/utils/adt/lockfuncs.c @@ -189,7 +189,7 @@ pg_lock_status(PG_FUNCTION_ARGS) granted = false; if (instance->holdMask) { - for (mode = 0; mode < MAX_LOCKMODES; mode++) + for (mode = 1; mode <= MAX_LOCKMODES; mode++) { if (instance->holdMask & LOCKBIT_ON(mode)) { -- 2.45.2