Skip to content

Commit 07ab138

Browse files
robertmhaasrobertmhaas
committed
Fix two more bugs in fast-path relation locking.
First, the previous code failed to account for the fact that, during Hot Standby operation, the startup process takes AccessExclusiveLocks on relations without setting MyDatabaseId. This resulted in fast path strong lock counts failing to be incremented with the startup process took locks, which in turn allowed conflicting lock requests to succeed when they should not have. Report by Erik Rijkers, diagnosis by Heikki Linnakangas. Second, LockReleaseAll() failed to honor the allLocks and lockmethodid restrictions with respect to fast-path locks. It's not clear to me whether this produces any user-visible breakage at the moment, but it's certainly wrong. Rearrange order of operations in LockReleaseAll to fix. Noted by Tom Lane.
1 parent 932ded2 commit 07ab138

File tree

1 file changed

+121
-114
lines changed
  • src/backend/storage/lmgr

1 file changed

+121
-114
lines changed

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

Lines changed: 121 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -192,14 +192,17 @@ static int FastPathLocalUseCount = 0;
192192
* self-conflicting, it can't use the fast-path mechanism; but it also does
193193
* not conflict with any of the locks that do, so we can ignore it completely.
194194
*/
195-
#define FastPathTag(locktag) \
195+
#define EligibleForRelationFastPath(locktag, mode) \
196196
((locktag)->locktag_lockmethodid == DEFAULT_LOCKMETHOD && \
197197
(locktag)->locktag_type == LOCKTAG_RELATION && \
198198
(locktag)->locktag_field1 == MyDatabaseId && \
199-
MyDatabaseId != InvalidOid)
200-
#define FastPathWeakMode(mode) ((mode) < ShareUpdateExclusiveLock)
201-
#define FastPathStrongMode(mode) ((mode) > ShareUpdateExclusiveLock)
202-
#define FastPathRelevantMode(mode) ((mode) != ShareUpdateExclusiveLock)
199+
MyDatabaseId != InvalidOid && \
200+
(mode) < ShareUpdateExclusiveLock)
201+
#define ConflictsWithRelationFastPath(locktag, mode) \
202+
((locktag)->locktag_lockmethodid == DEFAULT_LOCKMETHOD && \
203+
(locktag)->locktag_type == LOCKTAG_RELATION && \
204+
(locktag)->locktag_field1 != InvalidOid && \
205+
(mode) > ShareUpdateExclusiveLock)
203206

204207
static bool FastPathGrantRelationLock(Oid relid, LOCKMODE lockmode);
205208
static bool FastPathUnGrantRelationLock(Oid relid, LOCKMODE lockmode);
@@ -697,67 +700,71 @@ LockAcquireExtended(const LOCKTAG *locktag,
697700
log_lock = true;
698701
}
699702

700-
/* Locks that participate in the fast path require special handling. */
701-
if (FastPathTag(locktag) && FastPathRelevantMode(lockmode))
703+
/*
704+
* Attempt to take lock via fast path, if eligible. But if we remember
705+
* having filled up the fast path array, we don't attempt to make any
706+
* further use of it until we release some locks. It's possible that some
707+
* other backend has transferred some of those locks to the shared hash
708+
* table, leaving space free, but it's not worth acquiring the LWLock just
709+
* to check. It's also possible that we're acquiring a second or third
710+
* lock type on a relation we have already locked using the fast-path, but
711+
* for now we don't worry about that case either.
712+
*/
713+
if (EligibleForRelationFastPath(locktag, lockmode)
714+
&& FastPathLocalUseCount < FP_LOCK_SLOTS_PER_BACKEND)
702715
{
703-
uint32 fasthashcode;
704-
705-
fasthashcode = FastPathStrongLockHashPartition(hashcode);
716+
uint32 fasthashcode = FastPathStrongLockHashPartition(hashcode);
717+
bool acquired;
706718

707719
/*
708-
* If we remember having filled up the fast path array, we don't
709-
* attempt to make any further use of it until we release some locks.
710-
* It's possible that some other backend has transferred some of those
711-
* locks to the shared hash table, leaving space free, but it's not
712-
* worth acquiring the LWLock just to check. It's also possible that
713-
* we're acquiring a second or third lock type on a relation we have
714-
* already locked using the fast-path, but for now we don't worry about
715-
* that case either.
720+
* LWLockAcquire acts as a memory sequencing point, so it's safe
721+
* to assume that any strong locker whose increment to
722+
* FastPathStrongRelationLocks->counts becomes visible after we test
723+
* it has yet to begin to transfer fast-path locks.
716724
*/
717-
if (FastPathWeakMode(lockmode)
718-
&& FastPathLocalUseCount < FP_LOCK_SLOTS_PER_BACKEND)
725+
LWLockAcquire(MyProc->backendLock, LW_EXCLUSIVE);
726+
if (FastPathStrongRelationLocks->count[fasthashcode] != 0)
727+
acquired = false;
728+
else
729+
acquired = FastPathGrantRelationLock(locktag->locktag_field2,
730+
lockmode);
731+
LWLockRelease(MyProc->backendLock);
732+
if (acquired)
719733
{
720-
bool acquired;
721-
722-
/*
723-
* LWLockAcquire acts as a memory sequencing point, so it's safe
724-
* to assume that any strong locker whose increment to
725-
* FastPathStrongRelationLocks->counts becomes visible after we test
726-
* it has yet to begin to transfer fast-path locks.
727-
*/
728-
LWLockAcquire(MyProc->backendLock, LW_EXCLUSIVE);
729-
if (FastPathStrongRelationLocks->count[fasthashcode] != 0)
730-
acquired = false;
731-
else
732-
acquired = FastPathGrantRelationLock(locktag->locktag_field2,
733-
lockmode);
734-
LWLockRelease(MyProc->backendLock);
735-
if (acquired)
736-
{
737-
GrantLockLocal(locallock, owner);
738-
return LOCKACQUIRE_OK;
739-
}
734+
GrantLockLocal(locallock, owner);
735+
return LOCKACQUIRE_OK;
740736
}
741-
else if (FastPathStrongMode(lockmode))
737+
}
738+
739+
/*
740+
* If this lock could potentially have been taken via the fast-path by
741+
* some other backend, we must (temporarily) disable further use of the
742+
* fast-path for this lock tag, and migrate any locks already taken via
743+
* this method to the main lock table.
744+
*/
745+
if (ConflictsWithRelationFastPath(locktag, lockmode))
746+
{
747+
uint32 fasthashcode = FastPathStrongLockHashPartition(hashcode);
748+
749+
BeginStrongLockAcquire(locallock, fasthashcode);
750+
if (!FastPathTransferRelationLocks(lockMethodTable, locktag,
751+
hashcode))
742752
{
743-
BeginStrongLockAcquire(locallock, fasthashcode);
744-
if (!FastPathTransferRelationLocks(lockMethodTable, locktag,
745-
hashcode))
746-
{
747-
AbortStrongLockAcquire();
748-
if (reportMemoryError)
749-
ereport(ERROR,
750-
(errcode(ERRCODE_OUT_OF_MEMORY),
751-
errmsg("out of shared memory"),
752-
errhint("You might need to increase max_locks_per_transaction.")));
753-
else
754-
return LOCKACQUIRE_NOT_AVAIL;
755-
}
753+
AbortStrongLockAcquire();
754+
if (reportMemoryError)
755+
ereport(ERROR,
756+
(errcode(ERRCODE_OUT_OF_MEMORY),
757+
errmsg("out of shared memory"),
758+
errhint("You might need to increase max_locks_per_transaction.")));
759+
else
760+
return LOCKACQUIRE_NOT_AVAIL;
756761
}
757762
}
758763

759764
/*
760-
* Otherwise we've got to mess with the shared lock table.
765+
* We didn't find the lock in our LOCALLOCK table, and we didn't manage
766+
* to take it via the fast-path, either, so we've got to mess with the
767+
* shared lock table.
761768
*/
762769
partitionLock = LockHashPartitionLock(hashcode);
763770

@@ -1688,8 +1695,8 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock)
16881695
if (locallock->nLocks > 0)
16891696
return TRUE;
16901697

1691-
/* Locks that participate in the fast path require special handling. */
1692-
if (FastPathTag(locktag) && FastPathWeakMode(lockmode)
1698+
/* Attempt fast release of any lock eligible for the fast path. */
1699+
if (EligibleForRelationFastPath(locktag, lockmode)
16931700
&& FastPathLocalUseCount > 0)
16941701
{
16951702
bool released;
@@ -1729,7 +1736,7 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock)
17291736
PROCLOCKTAG proclocktag;
17301737
bool found;
17311738

1732-
Assert(FastPathTag(locktag) && FastPathWeakMode(lockmode));
1739+
Assert(EligibleForRelationFastPath(locktag, lockmode));
17331740
lock = (LOCK *) hash_search_with_hash_value(LockMethodLockHash,
17341741
(const void *) locktag,
17351742
locallock->hashcode,
@@ -1830,28 +1837,63 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
18301837

18311838
while ((locallock = (LOCALLOCK *) hash_seq_search(&status)) != NULL)
18321839
{
1833-
if (locallock->proclock == NULL || locallock->lock == NULL)
1840+
/*
1841+
* If the LOCALLOCK entry is unused, we must've run out of shared
1842+
* memory while trying to set up this lock. Just forget the local
1843+
* entry.
1844+
*/
1845+
if (locallock->nLocks == 0)
18341846
{
1835-
LOCKMODE lockmode = locallock->tag.mode;
1836-
Oid relid;
1847+
RemoveLocalLock(locallock);
1848+
continue;
1849+
}
18371850

1838-
/*
1839-
* If the LOCALLOCK entry is unused, we must've run out of shared
1840-
* memory while trying to set up this lock. Just forget the local
1841-
* entry.
1842-
*/
1843-
if (locallock->nLocks == 0)
1851+
/* Ignore items that are not of the lockmethod to be removed */
1852+
if (LOCALLOCK_LOCKMETHOD(*locallock) != lockmethodid)
1853+
continue;
1854+
1855+
/*
1856+
* If we are asked to release all locks, we can just zap the entry.
1857+
* Otherwise, must scan to see if there are session locks. We assume
1858+
* there is at most one lockOwners entry for session locks.
1859+
*/
1860+
if (!allLocks)
1861+
{
1862+
LOCALLOCKOWNER *lockOwners = locallock->lockOwners;
1863+
1864+
/* If it's above array position 0, move it down to 0 */
1865+
for (i = locallock->numLockOwners - 1; i > 0; i--)
18441866
{
1845-
RemoveLocalLock(locallock);
1867+
if (lockOwners[i].owner == NULL)
1868+
{
1869+
lockOwners[0] = lockOwners[i];
1870+
break;
1871+
}
1872+
}
1873+
1874+
if (locallock->numLockOwners > 0 &&
1875+
lockOwners[0].owner == NULL &&
1876+
lockOwners[0].nLocks > 0)
1877+
{
1878+
/* Fix the locallock to show just the session locks */
1879+
locallock->nLocks = lockOwners[0].nLocks;
1880+
locallock->numLockOwners = 1;
1881+
/* We aren't deleting this locallock, so done */
18461882
continue;
18471883
}
1884+
}
18481885

1849-
/*
1850-
* Otherwise, we should be dealing with a lock acquired via the
1851-
* fast-path. If not, we've got trouble.
1852-
*/
1853-
if (!FastPathTag(&locallock->tag.lock)
1854-
|| !FastPathWeakMode(lockmode))
1886+
/*
1887+
* If the lock or proclock pointers are NULL, this lock was taken via
1888+
* the relation fast-path.
1889+
*/
1890+
if (locallock->proclock == NULL || locallock->lock == NULL)
1891+
{
1892+
LOCKMODE lockmode = locallock->tag.mode;
1893+
Oid relid;
1894+
1895+
/* Verify that a fast-path lock is what we've got. */
1896+
if (!EligibleForRelationFastPath(&locallock->tag.lock, lockmode))
18551897
elog(PANIC, "locallock table corrupted");
18561898

18571899
/*
@@ -1894,41 +1936,6 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
18941936
continue;
18951937
}
18961938

1897-
/* Ignore items that are not of the lockmethod to be removed */
1898-
if (LOCALLOCK_LOCKMETHOD(*locallock) != lockmethodid)
1899-
continue;
1900-
1901-
/*
1902-
* If we are asked to release all locks, we can just zap the entry.
1903-
* Otherwise, must scan to see if there are session locks. We assume
1904-
* there is at most one lockOwners entry for session locks.
1905-
*/
1906-
if (!allLocks)
1907-
{
1908-
LOCALLOCKOWNER *lockOwners = locallock->lockOwners;
1909-
1910-
/* If it's above array position 0, move it down to 0 */
1911-
for (i = locallock->numLockOwners - 1; i > 0; i--)
1912-
{
1913-
if (lockOwners[i].owner == NULL)
1914-
{
1915-
lockOwners[0] = lockOwners[i];
1916-
break;
1917-
}
1918-
}
1919-
1920-
if (locallock->numLockOwners > 0 &&
1921-
lockOwners[0].owner == NULL &&
1922-
lockOwners[0].nLocks > 0)
1923-
{
1924-
/* Fix the locallock to show just the session locks */
1925-
locallock->nLocks = lockOwners[0].nLocks;
1926-
locallock->numLockOwners = 1;
1927-
/* We aren't deleting this locallock, so done */
1928-
continue;
1929-
}
1930-
}
1931-
19321939
/* Mark the proclock to show we need to release this lockmode */
19331940
if (locallock->nLocks > 0)
19341941
locallock->proclock->releaseMask |= LOCKBIT_ON(locallock->tag.mode);
@@ -2481,10 +2488,10 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode)
24812488

24822489
/*
24832490
* Fast path locks might not have been entered in the primary lock table.
2484-
* But only strong locks can conflict with anything that might have been
2485-
* taken via the fast-path mechanism.
2491+
* If the lock we're dealing with could conflict with such a lock, we must
2492+
* examine each backend's fast-path array for conflicts.
24862493
*/
2487-
if (FastPathTag(locktag) && FastPathStrongMode(lockmode))
2494+
if (ConflictsWithRelationFastPath(locktag, lockmode))
24882495
{
24892496
int i;
24902497
Oid relid = locktag->locktag_field2;
@@ -2722,7 +2729,7 @@ LockRefindAndRelease(LockMethod lockMethodTable, PGPROC *proc,
27222729
* Decrement strong lock count. This logic is needed only for 2PC.
27232730
*/
27242731
if (decrement_strong_lock_count
2725-
&& FastPathTag(&lock->tag) && FastPathStrongMode(lockmode))
2732+
&& ConflictsWithRelationFastPath(&lock->tag, lockmode))
27262733
{
27272734
uint32 fasthashcode = FastPathStrongLockHashPartition(hashcode);
27282735
SpinLockAcquire(&FastPathStrongRelationLocks->mutex);
@@ -3604,7 +3611,7 @@ lock_twophase_recover(TransactionId xid, uint16 info,
36043611
* Bump strong lock count, to make sure any fast-path lock requests won't
36053612
* be granted without consulting the primary lock table.
36063613
*/
3607-
if (FastPathTag(&lock->tag) && FastPathStrongMode(lockmode))
3614+
if (ConflictsWithRelationFastPath(&lock->tag, lockmode))
36083615
{
36093616
uint32 fasthashcode = FastPathStrongLockHashPartition(hashcode);
36103617
SpinLockAcquire(&FastPathStrongRelationLocks->mutex);

0 commit comments

Comments
 (0)