public inbox for [email protected]
help / color / mirror / Atom feedFrom: =?gb18030?B?SmluZ3hpYW4gTGk=?= <[email protected]>
To: =?gb18030?B?UG9zdGdyZVNRTCZuYnNwO0hhY2tlcnM=?= <[email protected]>
Subject: [PATCH] LockAcquireExtended improvement
Date: Tue, 28 Nov 2023 20:52:31 +0800
Message-ID: <[email protected]> (raw)
Hi hackers,
I found a problem when doing the test shown below:
Time
Session A
Session B
T1
postgres=# create table test(a int);
CREATE TABLE
postgres=# insert into test values (1);
INSERT 0 1
T2
postgres=# begin;
BEGIN
postgres=*# lock table test in access exclusive mode ;
LOCK TABLE
T3
postgres=# begin;
BEGIN
postgres=*# lock table test in exclusive mode ;
T4
Case 1:
postgres=*# lock table test in share row exclusive mode nowait;
ERROR: could not obtain lock on relation "test"
--------------------------------------------
Case 2:
postgres=*# lock table test in share row exclusive mode;
LOCK TABLE
At T4 moment in session A, (case 1) when executing SQL “lock table test in share row exclusive mode nowait;”, an error occurs with message “could not obtain lock on relation test";However, (case 2) when executing the SQL above without nowait, lock can be obtained successfully.
Digging into the source code, I find that in case 2 the lock was obtained in the function ProcSleep instead of LockAcquireExtended. Due to nowait logic processed before WaitOnLock->ProcSleep, acquiring lock failed in case 1. Can any changes be made so that the act of such lock granted occurs before WaitOnLock?
Providing a more universal case:
Transaction A already holds an n-mode lock on table test. If then transaction A requests an m-mode lock on table Test, m and n have the following constraints:
(lockMethodTable->conflictTab[n] & lockMethodTable->conflictTab[m]) == lockMethodTable->conflictTab[m]
Obviously, in this case, m<=n.
Should the m-mode lock be granted before WaitOnLock?
In the case of m=n (i.e. we already hold the lock), the m-mode lock is immediately granted in the LocalLock path, without the need of lock conflict check.
Based on the facts above, can we obtain a weaker lock (m<n) on the same object within the same transaction without doing lock conflict check?
Since m=n works, m<n should certainly work too.
I am attaching a patch here with which the problem in case 1 fixed.
With Regards,
Jingxian Li.
Attachments:
[application/octet-stream] v1-0001-LockAcquireExtended-improvement.patch (4.1K, ../[email protected]/3-v1-0001-LockAcquireExtended-improvement.patch)
download | inline diff:
From 137abc33199e91878b55fa054e0c409074e5856a Mon Sep 17 00:00:00 2001
From: Jingxian Li <[email protected]>
Date: Tue, 28 Nov 2023 16:01:48 +0800
Subject: [PATCH] LockAcquireExtended improvement
---
src/backend/storage/lmgr/lock.c | 66 +++++++++++++++++++++++++--------
1 file changed, 51 insertions(+), 15 deletions(-)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index b8c57b3e16..e0ce691941 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -357,6 +357,8 @@ PROCLOCK_PRINT(const char *where, const PROCLOCK *proclockP)
static uint32 proclock_hash(const void *key, Size keysize);
static void RemoveLocalLock(LOCALLOCK *locallock);
+static bool CheckLocalLockConflictTabCover(LockMethod lockMethodTable,
+ const LOCKTAG *locktag, LOCKMODE lockmode);
static PROCLOCK *SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc,
const LOCKTAG *locktag, uint32 hashcode, LOCKMODE lockmode);
static void GrantLockLocal(LOCALLOCK *locallock, ResourceOwner owner);
@@ -774,6 +776,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
uint32 hashcode;
LWLock *partitionLock;
bool found_conflict;
+ bool found_locallock_cover;
bool log_lock = false;
if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
@@ -938,6 +941,8 @@ LockAcquireExtended(const LOCKTAG *locktag,
}
}
+ found_locallock_cover = CheckLocalLockConflictTabCover(lockMethodTable, locktag, lockmode);
+
/*
* If this lock could potentially have been taken via the fast-path by
* some other backend, we must (temporarily) disable further use of the
@@ -949,21 +954,24 @@ LockAcquireExtended(const LOCKTAG *locktag,
uint32 fasthashcode = FastPathStrongLockHashPartition(hashcode);
BeginStrongLockAcquire(locallock, fasthashcode);
- if (!FastPathTransferRelationLocks(lockMethodTable, locktag,
- hashcode))
+ if (!found_locallock_cover)
{
- AbortStrongLockAcquire();
- if (locallock->nLocks == 0)
- RemoveLocalLock(locallock);
- if (locallockp)
- *locallockp = NULL;
- if (reportMemoryError)
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of shared memory"),
- errhint("You might need to increase %s.", "max_locks_per_transaction")));
- else
- return LOCKACQUIRE_NOT_AVAIL;
+ if (!FastPathTransferRelationLocks(lockMethodTable, locktag,
+ hashcode))
+ {
+ AbortStrongLockAcquire();
+ if (locallock->nLocks == 0)
+ RemoveLocalLock(locallock);
+ if (locallockp)
+ *locallockp = NULL;
+ if (reportMemoryError)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of shared memory"),
+ errhint("You might need to increase %s.", "max_locks_per_transaction")));
+ else
+ return LOCKACQUIRE_NOT_AVAIL;
+ }
}
}
@@ -1012,7 +1020,9 @@ LockAcquireExtended(const LOCKTAG *locktag,
* wait queue. Otherwise, check for conflict with already-held locks.
* (That's last because most complex check.)
*/
- if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
+ if (found_locallock_cover)
+ found_conflict = false;
+ else if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
found_conflict = true;
else
found_conflict = LockCheckConflicts(lockMethodTable, lockmode,
@@ -1137,6 +1147,32 @@ LockAcquireExtended(const LOCKTAG *locktag,
return LOCKACQUIRE_OK;
}
+/*
+ * CheckLocalLockConflictTabCover -- test if there is a local lock
+ * whose conflictTab covers the requested lock's
+ *
+ * Returns true if any such local lock found, false if not.
+ */
+static bool
+CheckLocalLockConflictTabCover(LockMethod lockMethodTable,
+ const LOCKTAG *locktag, LOCKMODE lockmode)
+{
+ int conflictMask = lockMethodTable->conflictTab[lockmode];
+
+ for (int i = lockMethodTable->numLockModes; i > lockmode; i--)
+ {
+ if ((lockMethodTable->conflictTab[i] & conflictMask) == conflictMask)
+ {
+ if (LockHeldByMe(locktag, (LOCKMODE) i))
+ {
+ return true;
+ }
+ }
+ }
+
+ return false;
+}
+
/*
* Find or create LOCK and PROCLOCK objects as needed for a new lock
* request.
--
2.37.1.windows.1
view thread (11+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH] LockAcquireExtended improvement
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox