[PATCH] LockAcquireExtended improvement
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:
v1-0001-LockAcquireExtended-improvement.patchapplication/octet-stream; charset=gb18030; name=v1-0001-LockAcquireExtended-improvement.patchDownload
From 137abc33199e91878b55fa054e0c409074e5856a Mon Sep 17 00:00:00 2001
From: Jingxian Li <aqktjcm@qq.com>
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
Hi,
On 2023-11-28 20:52:31 +0800, Jingxian Li wrote:
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?
I don't think that'd make sense - lock reordering is done to prevent deadlocks
and is quite expensive. Why should NOWAIT incur that cost?
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.
Sure - it'd not help anybody to wait for a lock we already hold - in fact it'd
create a lot of deadlocks.
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?
Perhaps. There's no inherent "lock strength" ordering for all locks though.
Greetings,
Andres Freund
Hi Andres, Thanks for your quick reply!
On 2023/11/29 0:51, Andres Freund wrote:
Hi,
On 2023-11-28 20:52:31 +0800, Jingxian Li wrote:
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?I don't think that'd make sense - lock reordering is done to prevent deadlocks
and is quite expensive. Why should NOWAIT incur that cost?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.Sure - it'd not help anybody to wait for a lock we already hold - in fact it'd
create a lot of deadlocks.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?Perhaps. There's no inherent "lock strength" ordering for all locks though.
I also noticed that there is no inherent "lock strength" orderingfor all locks.
So I use the following method in the code to determine the strength of the lock:
if (m<n &&(lockMethodTable->conflictTab[n] &
lockMethodTable->conflictTab[m]) == lockMethodTable->conflictTab[m])
then we can say that m-mode lock is weaker than n-mode lock.
Transaction A already holds an n-mode lock on table test,
that is, there is no locks held conflicting with the n-mode lock on table test,
If then transaction A requests an m-mode lock on table test,
as n's confilctTab covers m, it can be concluded that
there are no locks conflicting with the requested m-mode lock.
Greetings,
Andres Freund
With regards,
Jingxian Li
On Tue, 28 Nov 2023 at 18:23, Jingxian Li <aqktjcm@qq.com> wrote:
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.
I did not see any test added for this, should we add a test case for this?
Regards,
Vignesh
Hello Jingxian Li!
I agree with you that this behavior seems surprising. I don't think
it's quite a bug, more of a limitation. However, I think it would be
nice to fix it if we can find a good way to do that.
On Wed, Nov 29, 2023 at 10:43 PM Jingxian Li <aqktjcm@qq.com> wrote:
Transaction A already holds an n-mode lock on table test,
that is, there is no locks held conflicting with the n-mode lock on table test,
If then transaction A requests an m-mode lock on table test,
as n's confilctTab covers m, it can be concluded that
there are no locks conflicting with the requested m-mode lock.
This algorithm seems correct to me, but I think Andres is right to be
concerned about overhead. You're proposing to inject a call to
CheckLocalLockConflictTabCover() into the main code path of
LockAcquireExtended(), so practically every lock acquisition will pay
the cost of that function. And that function is not particularly
cheap: every call to LockHeldByMe is a hash table lookup. That sounds
pretty painful. If we could incur the overhead of this only when we
knew for certain that we would otherwise have to fail, that would be
more palatable, but doing it on every non-fastpath heavyweight lock
acquisition seems way too expensive.
Even aside from overhead, the approach the patch takes doesn't seem
quite right to me. As you noted, ProcSleep() has logic to jump the
queue if adding ourselves at the end would inevitably result in
deadlock, which is why your test case doesn't need to wait until
deadlock_timeout for the lock acquisition to succeed. But because that
logic happens in ProcSleep(), it's not reached in the NOWAIT case,
which means that it doesn't help any more once NOWAIT is specified. I
think that the right way to fix the problem would be to reach that
check even in the NOWAIT case, which could be done either by hoisting
some of the logic in ProcSleep() up into LockAcquireExtended(), or by
pushing the nowait flag down into ProcSleep() so that we can fail only
if we're definitely going to sleep. The former seems more elegant in
theory, but the latter looks easier to implement, at least at first
glance.
But the patch as proposed instead invents a new way of making the test
case work, not leveraging the existing logic and, I suspect, not
matching the behavior in all cases.
I also agree with Vignesh that a test case would be a good idea. It
would need to be an isolation test, since the regular regression
tester isn't powerful enough for this (at least, I don't see how to
make it work).
I hope that this input is helpful to you.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hello Robert,
Thank you for your advice. It is very helpful to me.
On 2024/1/16 3:07, Robert Haas wrote:
Hello Jingxian Li!
I agree with you that this behavior seems surprising. I don't think
it's quite a bug, more of a limitation. However, I think it would be
nice to fix it if we can find a good way to do that.On Wed, Nov 29, 2023 at 10:43 PM Jingxian Li <aqktjcm@qq.com> wrote:
Transaction A already holds an n-mode lock on table test,
that is, there is no locks held conflicting with the n-mode lock on table test,
If then transaction A requests an m-mode lock on table test,
as n's confilctTab covers m, it can be concluded that
there are no locks conflicting with the requested m-mode lock.This algorithm seems correct to me, but I think Andres is right to be
concerned about overhead. You're proposing to inject a call to
CheckLocalLockConflictTabCover() into the main code path of
LockAcquireExtended(), so practically every lock acquisition will pay
the cost of that function. And that function is not particularly
cheap: every call to LockHeldByMe is a hash table lookup. That sounds
pretty painful. If we could incur the overhead of this only when we
knew for certain that we would otherwise have to fail, that would be
more palatable, but doing it on every non-fastpath heavyweight lock
acquisition seems way too expensive.Even aside from overhead, the approach the patch takes doesn't seem
quite right to me. As you noted, ProcSleep() has logic to jump the
queue if adding ourselves at the end would inevitably result in
deadlock, which is why your test case doesn't need to wait until
deadlock_timeout for the lock acquisition to succeed. But because that
logic happens in ProcSleep(), it's not reached in the NOWAIT case,
which means that it doesn't help any more once NOWAIT is specified. I
think that the right way to fix the problem would be to reach that
check even in the NOWAIT case, which could be done either by hoisting
some of the logic in ProcSleep() up into LockAcquireExtended(), or by
pushing the nowait flag down into ProcSleep() so that we can fail only
if we're definitely going to sleep. The former seems more elegant in
theory, but the latter looks easier to implement, at least at first
glance.
According to what you said, I resubmitted a patch which splits the ProcSleep
logic into two parts, the former is responsible for inserting self to
WaitQueue,
the latter is responsible for deadlock detection and processing, and the
former part is directly called by LockAcquireExtended before nowait fails.
In this way the nowait case can also benefit from adjusting the insertion
order of WaitQueue.
But the patch as proposed instead invents a new way of making the test
case work, not leveraging the existing logic and, I suspect, not
matching the behavior in all cases.I also agree with Vignesh that a test case would be a good idea. It
would need to be an isolation test, since the regular regression
tester isn't powerful enough for this (at least, I don't see how to
make it work).
A test case was also added in the dir src/test/isolation.
Jingxian Li
Attachments:
v2-0001-LockAcquireExtended-improvement.patchapplication/octet-stream; name=v2-0001-LockAcquireExtended-improvement.patchDownload
From 0ebe90790020ef46b1f66aaf6d8522bdd9daf8d1 Mon Sep 17 00:00:00 2001
From: Jingxian Li <aqktjcm@qq.com>
Date: Thu, 1 Feb 2024 11:21:58 +0800
Subject: [PATCH] LockAcquireExtended improvement
---
src/backend/storage/lmgr/lock.c | 160 ++++++++++----------
src/backend/storage/lmgr/proc.c | 94 +++++++-----
src/include/storage/proc.h | 4 +-
src/test/isolation/expected/lock-nowait.out | 9 ++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/lock-nowait.spec | 28 ++++
6 files changed, 178 insertions(+), 118 deletions(-)
create mode 100644 src/test/isolation/expected/lock-nowait.out
create mode 100644 src/test/isolation/specs/lock-nowait.spec
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index c70a1adb9a..724fe6fa8c 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -362,7 +362,7 @@ static PROCLOCK *SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc,
static void GrantLockLocal(LOCALLOCK *locallock, ResourceOwner owner);
static void BeginStrongLockAcquire(LOCALLOCK *locallock, uint32 fasthashcode);
static void FinishStrongLockAcquire(void);
-static void WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner);
+static void WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool early_deadlock);
static void ReleaseLockIfHeld(LOCALLOCK *locallock, bool sessionLock);
static void LockReassignOwner(LOCALLOCK *locallock, ResourceOwner parent);
static bool UnGrantLock(LOCK *lock, LOCKMODE lockmode,
@@ -775,6 +775,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
LWLock *partitionLock;
bool found_conflict;
bool log_lock = false;
+ bool early_deadlock = false;
if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
elog(ERROR, "unrecognized lock method: %d", lockmethodid);
@@ -1027,88 +1028,96 @@ LockAcquireExtended(const LOCKTAG *locktag,
else
{
/*
- * We can't acquire the lock immediately. If caller specified no
- * blocking, remove useless table entries and return
- * LOCKACQUIRE_NOT_AVAIL without waiting.
+ * Set bitmask of locks this process already holds on this object.
*/
- if (dontWait)
+ MyProc->heldLocks = proclock->holdMask;
+
+ if (InsertSelfIntoWaitQueue(locallock, lockMethodTable, dontWait, &early_deadlock) == PROC_WAIT_STATUS_OK)
{
- AbortStrongLockAcquire();
- if (proclock->holdMask == 0)
+ GrantLock(lock, proclock, lockmode);
+ GrantLockLocal(locallock, owner);
+ }
+ else
+ {
+ /*
+ * We can't acquire the lock immediately. If caller specified no
+ * blocking, remove useless table entries and return
+ * LOCKACQUIRE_NOT_AVAIL without waiting.
+ */
+ if (dontWait)
{
- uint32 proclock_hashcode;
-
- proclock_hashcode = ProcLockHashCode(&proclock->tag, hashcode);
- dlist_delete(&proclock->lockLink);
- dlist_delete(&proclock->procLink);
- if (!hash_search_with_hash_value(LockMethodProcLockHash,
- &(proclock->tag),
- proclock_hashcode,
- HASH_REMOVE,
- NULL))
- elog(PANIC, "proclock table corrupted");
+ AbortStrongLockAcquire();
+ if (proclock->holdMask == 0)
+ {
+ uint32 proclock_hashcode;
+
+ proclock_hashcode = ProcLockHashCode(&proclock->tag, hashcode);
+ dlist_delete(&proclock->lockLink);
+ dlist_delete(&proclock->procLink);
+ if (!hash_search_with_hash_value(LockMethodProcLockHash,
+ &(proclock->tag),
+ proclock_hashcode,
+ HASH_REMOVE,
+ NULL))
+ elog(PANIC, "proclock table corrupted");
+ }
+ else
+ PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
+ lock->nRequested--;
+ lock->requested[lockmode]--;
+ LOCK_PRINT("LockAcquire: conditional lock failed", lock, lockmode);
+ Assert((lock->nRequested > 0) && (lock->requested[lockmode] >= 0));
+ Assert(lock->nGranted <= lock->nRequested);
+ LWLockRelease(partitionLock);
+ if (locallock->nLocks == 0)
+ RemoveLocalLock(locallock);
+ if (locallockp)
+ *locallockp = NULL;
+ return LOCKACQUIRE_NOT_AVAIL;
}
- else
- PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
- lock->nRequested--;
- lock->requested[lockmode]--;
- LOCK_PRINT("LockAcquire: conditional lock failed", lock, lockmode);
- Assert((lock->nRequested > 0) && (lock->requested[lockmode] >= 0));
- Assert(lock->nGranted <= lock->nRequested);
- LWLockRelease(partitionLock);
- if (locallock->nLocks == 0)
- RemoveLocalLock(locallock);
- if (locallockp)
- *locallockp = NULL;
- return LOCKACQUIRE_NOT_AVAIL;
- }
- /*
- * Set bitmask of locks this process already holds on this object.
- */
- MyProc->heldLocks = proclock->holdMask;
-
- /*
- * Sleep till someone wakes me up.
- */
+ /*
+ * Sleep till someone wakes me up.
+ */
- TRACE_POSTGRESQL_LOCK_WAIT_START(locktag->locktag_field1,
- locktag->locktag_field2,
- locktag->locktag_field3,
- locktag->locktag_field4,
- locktag->locktag_type,
- lockmode);
+ TRACE_POSTGRESQL_LOCK_WAIT_START(locktag->locktag_field1,
+ locktag->locktag_field2,
+ locktag->locktag_field3,
+ locktag->locktag_field4,
+ locktag->locktag_type,
+ lockmode);
- WaitOnLock(locallock, owner);
+ WaitOnLock(locallock, owner, early_deadlock);
- TRACE_POSTGRESQL_LOCK_WAIT_DONE(locktag->locktag_field1,
- locktag->locktag_field2,
- locktag->locktag_field3,
- locktag->locktag_field4,
- locktag->locktag_type,
- lockmode);
+ TRACE_POSTGRESQL_LOCK_WAIT_DONE(locktag->locktag_field1,
+ locktag->locktag_field2,
+ locktag->locktag_field3,
+ locktag->locktag_field4,
+ locktag->locktag_type,
+ lockmode);
- /*
- * NOTE: do not do any material change of state between here and
- * return. All required changes in locktable state must have been
- * done when the lock was granted to us --- see notes in WaitOnLock.
- */
+ /*
+ * NOTE: do not do any material change of state between here and
+ * return. All required changes in locktable state must have been
+ * done when the lock was granted to us --- see notes in WaitOnLock.
+ */
- /*
- * Check the proclock entry status, in case something in the ipc
- * communication doesn't work correctly.
- */
- if (!(proclock->holdMask & LOCKBIT_ON(lockmode)))
- {
- AbortStrongLockAcquire();
- PROCLOCK_PRINT("LockAcquire: INCONSISTENT", proclock);
- LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode);
- /* Should we retry ? */
- LWLockRelease(partitionLock);
- elog(ERROR, "LockAcquire failed");
+ /*
+ * Check the proclock entry status, in case something in the ipc
+ * communication doesn't work correctly.
+ */
+ if (!(proclock->holdMask & LOCKBIT_ON(lockmode)))
+ {
+ AbortStrongLockAcquire();
+ PROCLOCK_PRINT("LockAcquire: INCONSISTENT", proclock);
+ LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode);
+ /* Should we retry ? */
+ LWLockRelease(partitionLock);
+ elog(ERROR, "LockAcquire failed");
+ }
+ PROCLOCK_PRINT("LockAcquire: granted", proclock);
+ LOCK_PRINT("LockAcquire: granted", lock, lockmode);
}
- PROCLOCK_PRINT("LockAcquire: granted", proclock);
- LOCK_PRINT("LockAcquire: granted", lock, lockmode);
}
/*
@@ -1782,11 +1791,8 @@ MarkLockClear(LOCALLOCK *locallock)
* The appropriate partition lock must be held at entry.
*/
static void
-WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
+WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool early_deadlock)
{
- LOCKMETHODID lockmethodid = LOCALLOCK_LOCKMETHOD(*locallock);
- LockMethod lockMethodTable = LockMethods[lockmethodid];
-
LOCK_PRINT("WaitOnLock: sleeping on lock",
locallock->lock, locallock->tag.mode);
@@ -1815,7 +1821,7 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
*/
PG_TRY();
{
- if (ProcSleep(locallock, lockMethodTable) != PROC_WAIT_STATUS_OK)
+ if (ProcSleep(locallock, early_deadlock) != PROC_WAIT_STATUS_OK)
{
/*
* We failed as a result of a deadlock, see CheckDeadLock(). Quit
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index e5977548fe..309a19033c 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1018,39 +1018,18 @@ AuxiliaryPidGetProc(int pid)
return result;
}
-
/*
- * ProcSleep -- put a process to sleep on the specified lock
- *
- * Caller must have set MyProc->heldLocks to reflect locks already held
- * on the lockable object by this process (under all XIDs).
- *
- * The lock table's partition lock must be held at entry, and will be held
- * at exit.
- *
- * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR if not (deadlock).
- *
- * ASSUME: that no one will fiddle with the queue until after
- * we release the partition lock.
- *
- * NOTES: The process queue is now a priority queue for locking.
+ * InsertSelfIntoWaitQueue -- Insert self into queue if dontWait is false
*/
ProcWaitStatus
-ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
+InsertSelfIntoWaitQueue(LOCALLOCK *locallock, LockMethod lockMethodTable, bool dontWait, bool *early_deadlock)
{
LOCKMODE lockmode = locallock->tag.mode;
LOCK *lock = locallock->lock;
PROCLOCK *proclock = locallock->proclock;
- uint32 hashcode = locallock->hashcode;
- LWLock *partitionLock = LockHashPartitionLock(hashcode);
dclist_head *waitQueue = &lock->waitProcs;
PGPROC *insert_before = NULL;
LOCKMASK myHeldLocks = MyProc->heldLocks;
- TimestampTz standbyWaitStart = 0;
- bool early_deadlock = false;
- bool allow_autovacuum_cancel = true;
- bool logged_recovery_conflict = false;
- ProcWaitStatus myWaitStatus;
PGPROC *leader = MyProc->lockGroupLeader;
/*
@@ -1125,8 +1104,11 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
* a flag to check below, and break out of loop. Also,
* record deadlock info for later message.
*/
- RememberSimpleDeadLock(MyProc, lockmode, lock, proc);
- early_deadlock = true;
+ if (!dontWait)
+ {
+ RememberSimpleDeadLock(MyProc, lockmode, lock, proc);
+ *early_deadlock = true;
+ }
break;
}
/* I must go before this waiter. Check special case. */
@@ -1135,8 +1117,6 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
proclock))
{
/* Skip the wait and just grant myself the lock. */
- GrantLock(lock, proclock, lockmode);
- GrantAwaitedLock();
return PROC_WAIT_STATUS_OK;
}
@@ -1149,22 +1129,56 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
}
}
- /*
- * Insert self into queue, at the position determined above.
- */
- if (insert_before)
- dclist_insert_before(waitQueue, &insert_before->links, &MyProc->links);
- else
- dclist_push_tail(waitQueue, &MyProc->links);
+ if (!dontWait)
+ {
+ /*
+ * Insert self into queue, at the position determined above.
+ */
+ if (insert_before)
+ dclist_insert_before(waitQueue, &insert_before->links, &MyProc->links);
+ else
+ dclist_push_tail(waitQueue, &MyProc->links);
- lock->waitMask |= LOCKBIT_ON(lockmode);
+ lock->waitMask |= LOCKBIT_ON(lockmode);
- /* Set up wait information in PGPROC object, too */
- MyProc->waitLock = lock;
- MyProc->waitProcLock = proclock;
- MyProc->waitLockMode = lockmode;
+ /* Set up wait information in PGPROC object, too */
+ MyProc->waitLock = lock;
+ MyProc->waitProcLock = proclock;
+ MyProc->waitLockMode = lockmode;
- MyProc->waitStatus = PROC_WAIT_STATUS_WAITING;
+ MyProc->waitStatus = PROC_WAIT_STATUS_WAITING;
+ }
+ return PROC_WAIT_STATUS_WAITING;
+}
+
+
+/*
+ * ProcSleep -- put a process to sleep on the specified lock
+ *
+ * Caller must have set MyProc->heldLocks to reflect locks already held
+ * on the lockable object by this process (under all XIDs).
+ *
+ * The lock table's partition lock must be held at entry, and will be held
+ * at exit.
+ *
+ * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR if not (deadlock).
+ *
+ * ASSUME: that no one will fiddle with the queue until after
+ * we release the partition lock.
+ *
+ * NOTES: The process queue is now a priority queue for locking.
+ */
+ProcWaitStatus
+ProcSleep(LOCALLOCK *locallock, bool early_deadlock)
+{
+ LOCKMODE lockmode = locallock->tag.mode;
+ LOCK *lock = locallock->lock;
+ uint32 hashcode = locallock->hashcode;
+ LWLock *partitionLock = LockHashPartitionLock(hashcode);
+ TimestampTz standbyWaitStart = 0;
+ bool allow_autovacuum_cancel = true;
+ bool logged_recovery_conflict = false;
+ ProcWaitStatus myWaitStatus;
/*
* If we detected deadlock, give up without waiting. This must agree with
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 4bc226e36c..5e57e09180 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -449,7 +449,9 @@ extern int GetStartupBufferPinWaitBufId(void);
extern bool HaveNFreeProcs(int n, int *nfree);
extern void ProcReleaseLocks(bool isCommit);
-extern ProcWaitStatus ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable);
+extern ProcWaitStatus InsertSelfIntoWaitQueue(LOCALLOCK *locallock,
+ LockMethod lockMethodTable, bool dontWait, bool *early_deadlock);
+extern ProcWaitStatus ProcSleep(LOCALLOCK *locallock, bool early_deadlock);
extern void ProcWakeup(PGPROC *proc, ProcWaitStatus waitStatus);
extern void ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock);
extern void CheckDeadLockAlert(void);
diff --git a/src/test/isolation/expected/lock-nowait.out b/src/test/isolation/expected/lock-nowait.out
new file mode 100644
index 0000000000..2dc5aad6f0
--- /dev/null
+++ b/src/test/isolation/expected/lock-nowait.out
@@ -0,0 +1,9 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1a s2a s1b s1c s2c
+step s1a: LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE;
+step s2a: LOCK TABLE a1 IN EXCLUSIVE MODE; <waiting ...>
+step s1b: LOCK TABLE a1 IN SHARE ROW EXCLUSIVE MODE NOWAIT;
+step s1c: COMMIT;
+step s2a: <... completed>
+step s2c: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b2be88ead1..188fc04f85 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -111,3 +111,4 @@ test: serializable-parallel
test: serializable-parallel-2
test: serializable-parallel-3
test: matview-write-skew
+test: lock-nowait
diff --git a/src/test/isolation/specs/lock-nowait.spec b/src/test/isolation/specs/lock-nowait.spec
new file mode 100644
index 0000000000..aaa0779d8b
--- /dev/null
+++ b/src/test/isolation/specs/lock-nowait.spec
@@ -0,0 +1,28 @@
+# While requesting nowait lock, if the lock requested should
+# be inserted in front of some waiter, check to see if the lock
+# conflicts with already-held locks or the requests before
+# the waiter. If not, then just grant myself the requested
+# lock immediately. Test this scenario.
+
+setup
+{
+ CREATE TABLE a1 ();
+}
+
+teardown
+{
+ DROP TABLE a1;
+}
+
+session s1
+setup { BEGIN; }
+step s1a { LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE; }
+step s1b { LOCK TABLE a1 IN SHARE ROW EXCLUSIVE MODE NOWAIT; }
+step s1c { COMMIT; }
+
+session s2
+setup { BEGIN; }
+step s2a { LOCK TABLE a1 IN EXCLUSIVE MODE; }
+step s2c { COMMIT; }
+
+permutation s1a s2a s1b s1c s2c
--
2.37.1.windows.1
On Thu, Feb 1, 2024 at 2:16 AM Jingxian Li <aqktjcm@qq.com> wrote:
According to what you said, I resubmitted a patch which splits the ProcSleep
logic into two parts, the former is responsible for inserting self to
WaitQueue,
the latter is responsible for deadlock detection and processing, and the
former part is directly called by LockAcquireExtended before nowait fails.
In this way the nowait case can also benefit from adjusting the insertion
order of WaitQueue.
I don't have time for a full review of this patch right now
unfortunately, but just looking at it quickly:
- It will be helpful if you write a clear commit message. If it gets
committed, there is a high chance the committer will rewrite your
message, but in the meantime it will help understanding.
- The comment for InsertSelfIntoWaitQueue needs improvement. It is
only one line. And it says "Insert self into queue if dontWait is
false" but then someone will wonder why the function would ever be
called with dontWait = true.
- Between the comments and the commit message, the division of
responsibilities between InsertSelfIntoWaitQueue and ProcSleep needs
to be clearly explained. Right now I don't think it is.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hello Robert,
On 2024/2/2 5:05, Robert Haas wrote:
On Thu, Feb 1, 2024 at 2:16 AM Jingxian Li <aqktjcm@qq.com> wrote:
According to what you said, I resubmitted a patch which splits the ProcSleep
logic into two parts, the former is responsible for inserting self to
WaitQueue,
the latter is responsible for deadlock detection and processing, and the
former part is directly called by LockAcquireExtended before nowait fails.
In this way the nowait case can also benefit from adjusting the insertion
order of WaitQueue.I don't have time for a full review of this patch right now
unfortunately, but just looking at it quickly:- It will be helpful if you write a clear commit message. If it gets
committed, there is a high chance the committer will rewrite your
message, but in the meantime it will help understanding.- The comment for InsertSelfIntoWaitQueue needs improvement. It is
only one line. And it says "Insert self into queue if dontWait is
false" but then someone will wonder why the function would ever be
called with dontWait = true.- Between the comments and the commit message, the division of
responsibilities between InsertSelfIntoWaitQueue and ProcSleep needs
to be clearly explained. Right now I don't think it is.
Based on your comments above, I improve the commit message and comment for
InsertSelfIntoWaitQueue in new patch.
--
Jingxian Li
Hello Robert,
On 2024/2/2 5:05, Robert Haas wrote:
On Thu, Feb 1, 2024 at 2:16 AM Jingxian Li <aqktjcm@qq.com> wrote:
According to what you said, I resubmitted a patch which splits the ProcSleep
logic into two parts, the former is responsible for inserting self to
WaitQueue,
the latter is responsible for deadlock detection and processing, and the
former part is directly called by LockAcquireExtended before nowait fails.
In this way the nowait case can also benefit from adjusting the insertion
order of WaitQueue.I don't have time for a full review of this patch right now
unfortunately, but just looking at it quickly:- It will be helpful if you write a clear commit message. If it gets
committed, there is a high chance the committer will rewrite your
message, but in the meantime it will help understanding.- The comment for InsertSelfIntoWaitQueue needs improvement. It is
only one line. And it says "Insert self into queue if dontWait is
false" but then someone will wonder why the function would ever be
called with dontWait = true.- Between the comments and the commit message, the division of
responsibilities between InsertSelfIntoWaitQueue and ProcSleep needs
to be clearly explained. Right now I don't think it is.
Based on your comments above, I improve the commit message and comment for
InsertSelfIntoWaitQueue in new patch.
--
Jingxian Li
Attachments:
v2-0002-LockAcquireExtended-improvement.patchapplication/octet-stream; name=v2-0002-LockAcquireExtended-improvement.patchDownload
From 5753debac7bd34aaaaca6a3463b62793d85d15b5 Mon Sep 17 00:00:00 2001
From: Jingxian Li <aqktjcm@qq.com>
Date: Thu, 8 Feb 2024 18:07:27 +0800
Subject: [PATCH] When requesting a lock without nowait, the lock may be
granted in 4 ways before delaying deadlock detection: 1. locallock exists
(LockAcquireExtended) 2. fastpath eligible (LockAcquireExtended) 3. no
conflict with any existing or waiting lock request (LockAcquireExtended) 4.
before the process joins the lock's wait queue(ProcSleep)
But if we request a lock in nowait mode, only the first 3 ways can be reached. Because the nowait failure logic occurs before ProcSleep is called.
To fix the bug above, ProcSleep function should be split into two parts: the first part (named InsertSelfIntoWaitQueue) is responsible for checking if the lock can be grantedb in the process of determining the insertion point, and adding the lock-requesting process to the lock's wait queue when check fails and nowait not set, it shoud be called before nowait failure logic in LockAcquireExtended, in this way the nowait case can also have a change to acquire the lock in way 4. The second part (named ProcSleep) is just responsible for handling the deadlock, it is called after nowait logic failure as it was.
---
src/backend/storage/lmgr/lock.c | 160 ++++++++++----------
src/backend/storage/lmgr/proc.c | 88 +++++++----
src/include/storage/proc.h | 4 +-
src/test/isolation/expected/lock-nowait.out | 9 ++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/lock-nowait.spec | 28 ++++
6 files changed, 179 insertions(+), 111 deletions(-)
create mode 100644 src/test/isolation/expected/lock-nowait.out
create mode 100644 src/test/isolation/specs/lock-nowait.spec
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index c70a1adb9a..724fe6fa8c 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -362,7 +362,7 @@ static PROCLOCK *SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc,
static void GrantLockLocal(LOCALLOCK *locallock, ResourceOwner owner);
static void BeginStrongLockAcquire(LOCALLOCK *locallock, uint32 fasthashcode);
static void FinishStrongLockAcquire(void);
-static void WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner);
+static void WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool early_deadlock);
static void ReleaseLockIfHeld(LOCALLOCK *locallock, bool sessionLock);
static void LockReassignOwner(LOCALLOCK *locallock, ResourceOwner parent);
static bool UnGrantLock(LOCK *lock, LOCKMODE lockmode,
@@ -775,6 +775,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
LWLock *partitionLock;
bool found_conflict;
bool log_lock = false;
+ bool early_deadlock = false;
if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
elog(ERROR, "unrecognized lock method: %d", lockmethodid);
@@ -1027,88 +1028,96 @@ LockAcquireExtended(const LOCKTAG *locktag,
else
{
/*
- * We can't acquire the lock immediately. If caller specified no
- * blocking, remove useless table entries and return
- * LOCKACQUIRE_NOT_AVAIL without waiting.
+ * Set bitmask of locks this process already holds on this object.
*/
- if (dontWait)
+ MyProc->heldLocks = proclock->holdMask;
+
+ if (InsertSelfIntoWaitQueue(locallock, lockMethodTable, dontWait, &early_deadlock) == PROC_WAIT_STATUS_OK)
{
- AbortStrongLockAcquire();
- if (proclock->holdMask == 0)
+ GrantLock(lock, proclock, lockmode);
+ GrantLockLocal(locallock, owner);
+ }
+ else
+ {
+ /*
+ * We can't acquire the lock immediately. If caller specified no
+ * blocking, remove useless table entries and return
+ * LOCKACQUIRE_NOT_AVAIL without waiting.
+ */
+ if (dontWait)
{
- uint32 proclock_hashcode;
-
- proclock_hashcode = ProcLockHashCode(&proclock->tag, hashcode);
- dlist_delete(&proclock->lockLink);
- dlist_delete(&proclock->procLink);
- if (!hash_search_with_hash_value(LockMethodProcLockHash,
- &(proclock->tag),
- proclock_hashcode,
- HASH_REMOVE,
- NULL))
- elog(PANIC, "proclock table corrupted");
+ AbortStrongLockAcquire();
+ if (proclock->holdMask == 0)
+ {
+ uint32 proclock_hashcode;
+
+ proclock_hashcode = ProcLockHashCode(&proclock->tag, hashcode);
+ dlist_delete(&proclock->lockLink);
+ dlist_delete(&proclock->procLink);
+ if (!hash_search_with_hash_value(LockMethodProcLockHash,
+ &(proclock->tag),
+ proclock_hashcode,
+ HASH_REMOVE,
+ NULL))
+ elog(PANIC, "proclock table corrupted");
+ }
+ else
+ PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
+ lock->nRequested--;
+ lock->requested[lockmode]--;
+ LOCK_PRINT("LockAcquire: conditional lock failed", lock, lockmode);
+ Assert((lock->nRequested > 0) && (lock->requested[lockmode] >= 0));
+ Assert(lock->nGranted <= lock->nRequested);
+ LWLockRelease(partitionLock);
+ if (locallock->nLocks == 0)
+ RemoveLocalLock(locallock);
+ if (locallockp)
+ *locallockp = NULL;
+ return LOCKACQUIRE_NOT_AVAIL;
}
- else
- PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
- lock->nRequested--;
- lock->requested[lockmode]--;
- LOCK_PRINT("LockAcquire: conditional lock failed", lock, lockmode);
- Assert((lock->nRequested > 0) && (lock->requested[lockmode] >= 0));
- Assert(lock->nGranted <= lock->nRequested);
- LWLockRelease(partitionLock);
- if (locallock->nLocks == 0)
- RemoveLocalLock(locallock);
- if (locallockp)
- *locallockp = NULL;
- return LOCKACQUIRE_NOT_AVAIL;
- }
- /*
- * Set bitmask of locks this process already holds on this object.
- */
- MyProc->heldLocks = proclock->holdMask;
-
- /*
- * Sleep till someone wakes me up.
- */
+ /*
+ * Sleep till someone wakes me up.
+ */
- TRACE_POSTGRESQL_LOCK_WAIT_START(locktag->locktag_field1,
- locktag->locktag_field2,
- locktag->locktag_field3,
- locktag->locktag_field4,
- locktag->locktag_type,
- lockmode);
+ TRACE_POSTGRESQL_LOCK_WAIT_START(locktag->locktag_field1,
+ locktag->locktag_field2,
+ locktag->locktag_field3,
+ locktag->locktag_field4,
+ locktag->locktag_type,
+ lockmode);
- WaitOnLock(locallock, owner);
+ WaitOnLock(locallock, owner, early_deadlock);
- TRACE_POSTGRESQL_LOCK_WAIT_DONE(locktag->locktag_field1,
- locktag->locktag_field2,
- locktag->locktag_field3,
- locktag->locktag_field4,
- locktag->locktag_type,
- lockmode);
+ TRACE_POSTGRESQL_LOCK_WAIT_DONE(locktag->locktag_field1,
+ locktag->locktag_field2,
+ locktag->locktag_field3,
+ locktag->locktag_field4,
+ locktag->locktag_type,
+ lockmode);
- /*
- * NOTE: do not do any material change of state between here and
- * return. All required changes in locktable state must have been
- * done when the lock was granted to us --- see notes in WaitOnLock.
- */
+ /*
+ * NOTE: do not do any material change of state between here and
+ * return. All required changes in locktable state must have been
+ * done when the lock was granted to us --- see notes in WaitOnLock.
+ */
- /*
- * Check the proclock entry status, in case something in the ipc
- * communication doesn't work correctly.
- */
- if (!(proclock->holdMask & LOCKBIT_ON(lockmode)))
- {
- AbortStrongLockAcquire();
- PROCLOCK_PRINT("LockAcquire: INCONSISTENT", proclock);
- LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode);
- /* Should we retry ? */
- LWLockRelease(partitionLock);
- elog(ERROR, "LockAcquire failed");
+ /*
+ * Check the proclock entry status, in case something in the ipc
+ * communication doesn't work correctly.
+ */
+ if (!(proclock->holdMask & LOCKBIT_ON(lockmode)))
+ {
+ AbortStrongLockAcquire();
+ PROCLOCK_PRINT("LockAcquire: INCONSISTENT", proclock);
+ LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode);
+ /* Should we retry ? */
+ LWLockRelease(partitionLock);
+ elog(ERROR, "LockAcquire failed");
+ }
+ PROCLOCK_PRINT("LockAcquire: granted", proclock);
+ LOCK_PRINT("LockAcquire: granted", lock, lockmode);
}
- PROCLOCK_PRINT("LockAcquire: granted", proclock);
- LOCK_PRINT("LockAcquire: granted", lock, lockmode);
}
/*
@@ -1782,11 +1791,8 @@ MarkLockClear(LOCALLOCK *locallock)
* The appropriate partition lock must be held at entry.
*/
static void
-WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
+WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool early_deadlock)
{
- LOCKMETHODID lockmethodid = LOCALLOCK_LOCKMETHOD(*locallock);
- LockMethod lockMethodTable = LockMethods[lockmethodid];
-
LOCK_PRINT("WaitOnLock: sleeping on lock",
locallock->lock, locallock->tag.mode);
@@ -1815,7 +1821,7 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
*/
PG_TRY();
{
- if (ProcSleep(locallock, lockMethodTable) != PROC_WAIT_STATUS_OK)
+ if (ProcSleep(locallock, early_deadlock) != PROC_WAIT_STATUS_OK)
{
/*
* We failed as a result of a deadlock, see CheckDeadLock(). Quit
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index e5977548fe..f59f6e9e55 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1018,39 +1018,29 @@ AuxiliaryPidGetProc(int pid)
return result;
}
-
/*
- * ProcSleep -- put a process to sleep on the specified lock
+ * InsertSelfIntoWaitQueue -- Check if the lock can be grantedb in the
+ * process of determining the insertion point, and adding myProc to the
+ * lock's wait queue when check fails and dontWait was false.
*
* Caller must have set MyProc->heldLocks to reflect locks already held
* on the lockable object by this process (under all XIDs).
*
- * The lock table's partition lock must be held at entry, and will be held
- * at exit.
+ * The lock table's partition lock must be held at entry.
*
- * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR if not (deadlock).
- *
- * ASSUME: that no one will fiddle with the queue until after
- * we release the partition lock.
+ * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_WAITING if not.
*
* NOTES: The process queue is now a priority queue for locking.
*/
ProcWaitStatus
-ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
+InsertSelfIntoWaitQueue(LOCALLOCK *locallock, LockMethod lockMethodTable, bool dontWait, bool *early_deadlock)
{
LOCKMODE lockmode = locallock->tag.mode;
LOCK *lock = locallock->lock;
PROCLOCK *proclock = locallock->proclock;
- uint32 hashcode = locallock->hashcode;
- LWLock *partitionLock = LockHashPartitionLock(hashcode);
dclist_head *waitQueue = &lock->waitProcs;
PGPROC *insert_before = NULL;
LOCKMASK myHeldLocks = MyProc->heldLocks;
- TimestampTz standbyWaitStart = 0;
- bool early_deadlock = false;
- bool allow_autovacuum_cancel = true;
- bool logged_recovery_conflict = false;
- ProcWaitStatus myWaitStatus;
PGPROC *leader = MyProc->lockGroupLeader;
/*
@@ -1125,8 +1115,11 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
* a flag to check below, and break out of loop. Also,
* record deadlock info for later message.
*/
- RememberSimpleDeadLock(MyProc, lockmode, lock, proc);
- early_deadlock = true;
+ if (!dontWait)
+ {
+ RememberSimpleDeadLock(MyProc, lockmode, lock, proc);
+ *early_deadlock = true;
+ }
break;
}
/* I must go before this waiter. Check special case. */
@@ -1135,8 +1128,6 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
proclock))
{
/* Skip the wait and just grant myself the lock. */
- GrantLock(lock, proclock, lockmode);
- GrantAwaitedLock();
return PROC_WAIT_STATUS_OK;
}
@@ -1149,22 +1140,53 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
}
}
- /*
- * Insert self into queue, at the position determined above.
- */
- if (insert_before)
- dclist_insert_before(waitQueue, &insert_before->links, &MyProc->links);
- else
- dclist_push_tail(waitQueue, &MyProc->links);
+ if (!dontWait)
+ {
+ /*
+ * Insert self into queue, at the position determined above.
+ */
+ if (insert_before)
+ dclist_insert_before(waitQueue, &insert_before->links, &MyProc->links);
+ else
+ dclist_push_tail(waitQueue, &MyProc->links);
+
+ lock->waitMask |= LOCKBIT_ON(lockmode);
+
+ /* Set up wait information in PGPROC object, too */
+ MyProc->waitLock = lock;
+ MyProc->waitProcLock = proclock;
+ MyProc->waitLockMode = lockmode;
- lock->waitMask |= LOCKBIT_ON(lockmode);
+ MyProc->waitStatus = PROC_WAIT_STATUS_WAITING;
+ }
+ return PROC_WAIT_STATUS_WAITING;
+}
- /* Set up wait information in PGPROC object, too */
- MyProc->waitLock = lock;
- MyProc->waitProcLock = proclock;
- MyProc->waitLockMode = lockmode;
- MyProc->waitStatus = PROC_WAIT_STATUS_WAITING;
+/*
+ * ProcSleep -- put a process to sleep on the specified lock
+ *
+ * The lock table's partition lock must be held at entry, and will be held
+ * at exit.
+ *
+ * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR if not (deadlock).
+ *
+ * ASSUME: that no one will fiddle with the queue until after
+ * we release the partition lock.
+ *
+ * NOTES: The process queue is now a priority queue for locking.
+ */
+ProcWaitStatus
+ProcSleep(LOCALLOCK *locallock, bool early_deadlock)
+{
+ LOCKMODE lockmode = locallock->tag.mode;
+ LOCK *lock = locallock->lock;
+ uint32 hashcode = locallock->hashcode;
+ LWLock *partitionLock = LockHashPartitionLock(hashcode);
+ TimestampTz standbyWaitStart = 0;
+ bool allow_autovacuum_cancel = true;
+ bool logged_recovery_conflict = false;
+ ProcWaitStatus myWaitStatus;
/*
* If we detected deadlock, give up without waiting. This must agree with
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 4bc226e36c..5e57e09180 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -449,7 +449,9 @@ extern int GetStartupBufferPinWaitBufId(void);
extern bool HaveNFreeProcs(int n, int *nfree);
extern void ProcReleaseLocks(bool isCommit);
-extern ProcWaitStatus ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable);
+extern ProcWaitStatus InsertSelfIntoWaitQueue(LOCALLOCK *locallock,
+ LockMethod lockMethodTable, bool dontWait, bool *early_deadlock);
+extern ProcWaitStatus ProcSleep(LOCALLOCK *locallock, bool early_deadlock);
extern void ProcWakeup(PGPROC *proc, ProcWaitStatus waitStatus);
extern void ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock);
extern void CheckDeadLockAlert(void);
diff --git a/src/test/isolation/expected/lock-nowait.out b/src/test/isolation/expected/lock-nowait.out
new file mode 100644
index 0000000000..2dc5aad6f0
--- /dev/null
+++ b/src/test/isolation/expected/lock-nowait.out
@@ -0,0 +1,9 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1a s2a s1b s1c s2c
+step s1a: LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE;
+step s2a: LOCK TABLE a1 IN EXCLUSIVE MODE; <waiting ...>
+step s1b: LOCK TABLE a1 IN SHARE ROW EXCLUSIVE MODE NOWAIT;
+step s1c: COMMIT;
+step s2a: <... completed>
+step s2c: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b2be88ead1..188fc04f85 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -111,3 +111,4 @@ test: serializable-parallel
test: serializable-parallel-2
test: serializable-parallel-3
test: matview-write-skew
+test: lock-nowait
diff --git a/src/test/isolation/specs/lock-nowait.spec b/src/test/isolation/specs/lock-nowait.spec
new file mode 100644
index 0000000000..aaa0779d8b
--- /dev/null
+++ b/src/test/isolation/specs/lock-nowait.spec
@@ -0,0 +1,28 @@
+# While requesting nowait lock, if the lock requested should
+# be inserted in front of some waiter, check to see if the lock
+# conflicts with already-held locks or the requests before
+# the waiter. If not, then just grant myself the requested
+# lock immediately. Test this scenario.
+
+setup
+{
+ CREATE TABLE a1 ();
+}
+
+teardown
+{
+ DROP TABLE a1;
+}
+
+session s1
+setup { BEGIN; }
+step s1a { LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE; }
+step s1b { LOCK TABLE a1 IN SHARE ROW EXCLUSIVE MODE NOWAIT; }
+step s1c { COMMIT; }
+
+session s2
+setup { BEGIN; }
+step s2a { LOCK TABLE a1 IN EXCLUSIVE MODE; }
+step s2c { COMMIT; }
+
+permutation s1a s2a s1b s1c s2c
--
2.37.1.windows.1
On Thu, Feb 8, 2024 at 5:28 AM Jingxian Li <aqktjcm@qq.com> wrote:
Based on your comments above, I improve the commit message and comment for
InsertSelfIntoWaitQueue in new patch.
Well, I had a look at this patch today, and even after reading the new
commit message, I couldn't really convince myself that it was correct.
It may well be entirely correct, but I simply find it hard to tell. It
would help if the comments had been adjusted a bit more, e.g.
/* Skip the wait and just
grant myself the lock. */
- GrantLock(lock, proclock, lockmode);
- GrantAwaitedLock();
return PROC_WAIT_STATUS_OK;
Surely this is not an acceptable change. The comments says "and just
grant myself the lock" but the code no longer does that.
But instead of just complaining, I decided to try writing a version of
the patch that seemed acceptable to me. Here it is. I took a different
approach than you. Instead of splitting up ProcSleep(), I just passed
down the dontWait flag to WaitOnLock() and ProcSleep(). In
LockAcquireExtended(), I moved the existing code that handles giving
up in the don't-wait case from before the call to ProcSleep() to
afterward. As far as I can see, the major way this could be wrong is
if calling ProcSleep() with dontWait = true and having it fail to
acquire the lock changes the state in some way that makes the cleanup
code that I moved incorrect. I don't *think* that's the case, but I
might be wrong.
What do you think of this version?
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v3-0001-Allow-a-no-wait-lock-acquisition-to-succeed-in-mo.patchapplication/octet-stream; name=v3-0001-Allow-a-no-wait-lock-acquisition-to-succeed-in-mo.patchDownload
From 3de8e3ec4984177fd95ab897eb68920da735b996 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 7 Mar 2024 11:36:58 -0500
Subject: [PATCH v3] Allow a no-wait lock acquisition to succeed in more cases.
We don't determine the position at which a process waiting for a lock
should insert itself into the wait queue until we reach ProcSleep(),
and we may at that point discover that we must insert ourselves ahead
of everyone who wants a conflicting lock, in which case we obtain the
lock immediately. Up until now, a no-wait lock acquisition would fail
in such cases, erroneously claiming that the lock couldn't be obtained
immediately. Fix that by trying ProcSleep even in the no-wait case.
No back-patch for now, because I'm treating this as an improvement to
the existing no-wait feature. It could instead be argued that it's a
bug fix, on the theory that there should never be any case whatsoever
where no-wait fails to obtain a lock that would have been obtained
immediately without no-wait, but I'm reluctant to interpret the
semantics of no-wait that strictly.
---
src/backend/storage/lmgr/lock.c | 119 ++++++++++++--------
src/backend/storage/lmgr/proc.c | 20 +++-
src/include/storage/proc.h | 4 +-
src/test/isolation/expected/lock-nowait.out | 9 ++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/lock-nowait.spec | 28 +++++
6 files changed, 128 insertions(+), 53 deletions(-)
create mode 100644 src/test/isolation/expected/lock-nowait.out
create mode 100644 src/test/isolation/specs/lock-nowait.spec
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 0d93932d8d..5022a50dd7 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -360,7 +360,8 @@ static PROCLOCK *SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc,
static void GrantLockLocal(LOCALLOCK *locallock, ResourceOwner owner);
static void BeginStrongLockAcquire(LOCALLOCK *locallock, uint32 fasthashcode);
static void FinishStrongLockAcquire(void);
-static void WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner);
+static void WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner,
+ bool dontWait);
static void ReleaseLockIfHeld(LOCALLOCK *locallock, bool sessionLock);
static void LockReassignOwner(LOCALLOCK *locallock, ResourceOwner parent);
static bool UnGrantLock(LOCK *lock, LOCKMODE lockmode,
@@ -1024,50 +1025,15 @@ LockAcquireExtended(const LOCKTAG *locktag,
}
else
{
- /*
- * We can't acquire the lock immediately. If caller specified no
- * blocking, remove useless table entries and return
- * LOCKACQUIRE_NOT_AVAIL without waiting.
- */
- if (dontWait)
- {
- AbortStrongLockAcquire();
- if (proclock->holdMask == 0)
- {
- uint32 proclock_hashcode;
-
- proclock_hashcode = ProcLockHashCode(&proclock->tag, hashcode);
- dlist_delete(&proclock->lockLink);
- dlist_delete(&proclock->procLink);
- if (!hash_search_with_hash_value(LockMethodProcLockHash,
- &(proclock->tag),
- proclock_hashcode,
- HASH_REMOVE,
- NULL))
- elog(PANIC, "proclock table corrupted");
- }
- else
- PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
- lock->nRequested--;
- lock->requested[lockmode]--;
- LOCK_PRINT("LockAcquire: conditional lock failed", lock, lockmode);
- Assert((lock->nRequested > 0) && (lock->requested[lockmode] >= 0));
- Assert(lock->nGranted <= lock->nRequested);
- LWLockRelease(partitionLock);
- if (locallock->nLocks == 0)
- RemoveLocalLock(locallock);
- if (locallockp)
- *locallockp = NULL;
- return LOCKACQUIRE_NOT_AVAIL;
- }
-
/*
* Set bitmask of locks this process already holds on this object.
*/
MyProc->heldLocks = proclock->holdMask;
/*
- * Sleep till someone wakes me up.
+ * Sleep till someone wakes me up. We do this even in the dontWait
+ * case, beause while trying to go to sleep, we may discover that we
+ * can acquire the lock immediately after all.
*/
TRACE_POSTGRESQL_LOCK_WAIT_START(locktag->locktag_field1,
@@ -1077,7 +1043,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
locktag->locktag_type,
lockmode);
- WaitOnLock(locallock, owner);
+ WaitOnLock(locallock, owner, dontWait);
TRACE_POSTGRESQL_LOCK_WAIT_DONE(locktag->locktag_field1,
locktag->locktag_field2,
@@ -1093,17 +1059,63 @@ LockAcquireExtended(const LOCKTAG *locktag,
*/
/*
- * Check the proclock entry status, in case something in the ipc
- * communication doesn't work correctly.
+ * Check the proclock entry status. If dontWait = true, this is an
+ * expected case; otherwise, it will open happen if something in the
+ * ipc communication doesn't work correctly.
*/
if (!(proclock->holdMask & LOCKBIT_ON(lockmode)))
{
AbortStrongLockAcquire();
- PROCLOCK_PRINT("LockAcquire: INCONSISTENT", proclock);
- LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode);
- /* Should we retry ? */
- LWLockRelease(partitionLock);
- elog(ERROR, "LockAcquire failed");
+
+ if (dontWait)
+ {
+ /*
+ * We can't acquire the lock immediately. If caller specified
+ * no blocking, remove useless table entries and return
+ * LOCKACQUIRE_NOT_AVAIL without waiting.
+ */
+ if (proclock->holdMask == 0)
+ {
+ uint32 proclock_hashcode;
+
+ proclock_hashcode = ProcLockHashCode(&proclock->tag,
+ hashcode);
+ dlist_delete(&proclock->lockLink);
+ dlist_delete(&proclock->procLink);
+ if (!hash_search_with_hash_value(LockMethodProcLockHash,
+ &(proclock->tag),
+ proclock_hashcode,
+ HASH_REMOVE,
+ NULL))
+ elog(PANIC, "proclock table corrupted");
+ }
+ else
+ PROCLOCK_PRINT("LockAcquire: NOWAIT", proclock);
+ lock->nRequested--;
+ lock->requested[lockmode]--;
+ LOCK_PRINT("LockAcquire: conditional lock failed",
+ lock, lockmode);
+ Assert((lock->nRequested > 0) &&
+ (lock->requested[lockmode] >= 0));
+ Assert(lock->nGranted <= lock->nRequested);
+ LWLockRelease(partitionLock);
+ if (locallock->nLocks == 0)
+ RemoveLocalLock(locallock);
+ if (locallockp)
+ *locallockp = NULL;
+ return LOCKACQUIRE_NOT_AVAIL;
+ }
+ else
+ {
+ /*
+ * We should have gotten the lock, but somehow that didn't
+ * happen. If we get here, it's a bug.
+ */
+ PROCLOCK_PRINT("LockAcquire: INCONSISTENT", proclock);
+ LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode);
+ LWLockRelease(partitionLock);
+ elog(ERROR, "LockAcquire failed");
+ }
}
PROCLOCK_PRINT("LockAcquire: granted", proclock);
LOCK_PRINT("LockAcquire: granted", lock, lockmode);
@@ -1777,10 +1789,11 @@ MarkLockClear(LOCALLOCK *locallock)
* Caller must have set MyProc->heldLocks to reflect locks already held
* on the lockable object by this process.
*
- * The appropriate partition lock must be held at entry.
+ * The appropriate partition lock must be held at entry, and will still be
+ * held at exit.
*/
static void
-WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
+WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool dontWait)
{
LOCKMETHODID lockmethodid = LOCALLOCK_LOCKMETHOD(*locallock);
LockMethod lockMethodTable = LockMethods[lockmethodid];
@@ -1813,8 +1826,14 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
*/
PG_TRY();
{
- if (ProcSleep(locallock, lockMethodTable) != PROC_WAIT_STATUS_OK)
+ /*
+ * If dontWait = true, we handle success and failure in the same way
+ * here. The caller will be able to sort out what has happened.
+ */
+ if (ProcSleep(locallock, lockMethodTable, dontWait) != PROC_WAIT_STATUS_OK
+ && !dontWait)
{
+
/*
* We failed as a result of a deadlock, see CheckDeadLock(). Quit
* now.
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index f3e20038f4..162b1f919d 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1043,10 +1043,19 @@ AuxiliaryPidGetProc(int pid)
* Caller must have set MyProc->heldLocks to reflect locks already held
* on the lockable object by this process (under all XIDs).
*
+ * It's not actually guaranteed that we need to wait when this function is
+ * called, because it could be that when we try to find a position at which
+ * to insert ourself into the wait queue, we discover that we must be inserted
+ * ahead of everyone who wants a lock that conflict with ours. In that case,
+ * we get the lock immediately. Beause of this, it's sensible for this function
+ * to have a dontWait argument, despite the name.
+ *
* The lock table's partition lock must be held at entry, and will be held
* at exit.
*
- * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR if not (deadlock).
+ * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR
+ * if not (if dontWait = true, this is a deadlock; if dontWait = false, we
+ * would have had to wait).
*
* ASSUME: that no one will fiddle with the queue until after
* we release the partition lock.
@@ -1054,7 +1063,7 @@ AuxiliaryPidGetProc(int pid)
* NOTES: The process queue is now a priority queue for locking.
*/
ProcWaitStatus
-ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
+ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable, bool dontWait)
{
LOCKMODE lockmode = locallock->tag.mode;
LOCK *lock = locallock->lock;
@@ -1167,6 +1176,13 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
}
}
+ /*
+ * At this point we know that we'd really need to sleep. If we've been
+ * commanded not to do that, bail out.
+ */
+ if (dontWait)
+ return PROC_WAIT_STATUS_ERROR;
+
/*
* Insert self into queue, at the position determined above.
*/
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 1095aefddf..18891a86fb 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -471,7 +471,9 @@ extern int GetStartupBufferPinWaitBufId(void);
extern bool HaveNFreeProcs(int n, int *nfree);
extern void ProcReleaseLocks(bool isCommit);
-extern ProcWaitStatus ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable);
+extern ProcWaitStatus ProcSleep(LOCALLOCK *locallock,
+ LockMethod lockMethodTable,
+ bool dontWait);
extern void ProcWakeup(PGPROC *proc, ProcWaitStatus waitStatus);
extern void ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock);
extern void CheckDeadLockAlert(void);
diff --git a/src/test/isolation/expected/lock-nowait.out b/src/test/isolation/expected/lock-nowait.out
new file mode 100644
index 0000000000..2dc5aad6f0
--- /dev/null
+++ b/src/test/isolation/expected/lock-nowait.out
@@ -0,0 +1,9 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1a s2a s1b s1c s2c
+step s1a: LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE;
+step s2a: LOCK TABLE a1 IN EXCLUSIVE MODE; <waiting ...>
+step s1b: LOCK TABLE a1 IN SHARE ROW EXCLUSIVE MODE NOWAIT;
+step s1c: COMMIT;
+step s2a: <... completed>
+step s2c: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b2be88ead1..188fc04f85 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -111,3 +111,4 @@ test: serializable-parallel
test: serializable-parallel-2
test: serializable-parallel-3
test: matview-write-skew
+test: lock-nowait
diff --git a/src/test/isolation/specs/lock-nowait.spec b/src/test/isolation/specs/lock-nowait.spec
new file mode 100644
index 0000000000..bb46d12a79
--- /dev/null
+++ b/src/test/isolation/specs/lock-nowait.spec
@@ -0,0 +1,28 @@
+# While requesting nowait lock, if the lock requested should
+# be inserted in front of some waiter, check to see if the lock
+# conflicts with already-held locks or the requests before
+# the waiter. If not, then just grant myself the requested
+# lock immediately. Test this scenario.
+
+setup
+{
+ CREATE TABLE a1 ();
+}
+
+teardown
+{
+ DROP TABLE a1;
+}
+
+session s1
+setup { BEGIN; }
+step s1a { LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE; }
+step s1b { LOCK TABLE a1 IN SHARE ROW EXCLUSIVE MODE NOWAIT; }
+step s1c { COMMIT; }
+
+session s2
+setup { BEGIN; }
+step s2a { LOCK TABLE a1 IN EXCLUSIVE MODE; }
+step s2c { COMMIT; }
+
+permutation s1a s2a s1b s1c s2c
--
2.39.3 (Apple Git-145)
Hello Robert,
On 2024/3/8 1:02, Robert Haas wrote:
But instead of just complaining, I decided to try writing a version of
the patch that seemed acceptable to me. Here it is. I took a different
approach than you. Instead of splitting up ProcSleep(), I just passed
down the dontWait flag to WaitOnLock() and ProcSleep(). In
LockAcquireExtended(), I moved the existing code that handles giving
up in the don't-wait case from before the call to ProcSleep() to
afterward. As far as I can see, the major way this could be wrong is
if calling ProcSleep() with dontWait = true and having it fail to
acquire the lock changes the state in some way that makes the cleanup
code that I moved incorrect. I don't *think* that's the case, but I
might be wrong.What do you think of this version?
Your version changes less code than mine by pushing the nowait flag down
into ProcSleep(). This looks fine in general, except for a little advice,
which I don't think there is necessary to add 'waiting' suffix to the
process name in function WaitOnLock with dontwait being true, as follows:
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -1801,8 +1801,12 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool dontWait)
LOCK_PRINT("WaitOnLock: sleeping on lock",
locallock->lock, locallock->tag.mode);- /* adjust the process title to indicate that it's waiting */
- set_ps_display_suffix("waiting");
+ if (!dontWait)
+ {
+ /* adjust the process title to indicate that it's waiting */
+ set_ps_display_suffix("waiting");
+ }
+ awaitedLock = locallock;
awaitedOwner = owner;
@@ -1855,9 +1859,12 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool dontWait)
{
/* In this path, awaitedLock remains set until LockErrorCleanup */
- /* reset ps display to remove the suffix */
- set_ps_display_remove_suffix();
-
+ if (!dontWait)
+ {
+ /* reset ps display to remove the suffix */
+ set_ps_display_remove_suffix();
+ }
+
/* and propagate the error */
PG_RE_THROW();
}
@@ -1865,8 +1872,11 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool dontWait)awaitedLock = NULL;
- /* reset ps display to remove the suffix */
- set_ps_display_remove_suffix();
+ if (!dontWait)
+ {
+ /* reset ps display to remove the suffix */
+ set_ps_display_remove_suffix();
+ } LOCK_PRINT("WaitOnLock: wakeup on lock",
locallock->lock, locallock->tag.mode);
--
Jingxian Li
On Mon, Mar 11, 2024 at 11:11 PM Jingxian Li <aqktjcm@qq.com> wrote:
Your version changes less code than mine by pushing the nowait flag down
into ProcSleep(). This looks fine in general, except for a little advice,
which I don't think there is necessary to add 'waiting' suffix to the
process name in function WaitOnLock with dontwait being true, as follows:
That could be done, but in my opinion it's not necessary. The waiting
suffix will appear only very briefly, and probably only in relatively
rare cases. It doesn't seem worth adding code to avoid it.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, Mar 12, 2024 at 9:33 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Mar 11, 2024 at 11:11 PM Jingxian Li <aqktjcm@qq.com> wrote:
Your version changes less code than mine by pushing the nowait flag down
into ProcSleep(). This looks fine in general, except for a little advice,
which I don't think there is necessary to add 'waiting' suffix to the
process name in function WaitOnLock with dontwait being true, as follows:That could be done, but in my opinion it's not necessary. The waiting
suffix will appear only very briefly, and probably only in relatively
rare cases. It doesn't seem worth adding code to avoid it.
Seeing no further discussion, I have committed my version of this
patch, with your test case.
Thanks for pursuing this improvement!
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Mar 14, 2024 at 1:15 PM Robert Haas <robertmhaas@gmail.com> wrote:
Seeing no further discussion, I have committed my version of this
patch, with your test case.
This comment on ProcSleep() seems to have the values of dontWait
backward (double negatives are tricky):
* Result: PROC_WAIT_STATUS_OK if we acquired the lock,
PROC_WAIT_STATUS_ERROR
* if not (if dontWait = true, this is a deadlock; if dontWait = false, we
* would have had to wait).
Also there's a minor typo in a comment in LockAcquireExtended():
* Check the proclock entry status. If dontWait = true, this is an
* expected case; otherwise, it will open happen if something in the
* ipc communication doesn't work correctly.
"open" should be "only".
On Tue, Mar 26, 2024 at 7:14 PM Will Mortensen <will@extrahop.com> wrote:
This comment on ProcSleep() seems to have the values of dontWait
backward (double negatives are tricky):* Result: PROC_WAIT_STATUS_OK if we acquired the lock,
PROC_WAIT_STATUS_ERROR
* if not (if dontWait = true, this is a deadlock; if dontWait = false, we
* would have had to wait).Also there's a minor typo in a comment in LockAcquireExtended():
* Check the proclock entry status. If dontWait = true, this is an
* expected case; otherwise, it will open happen if something in the
* ipc communication doesn't work correctly."open" should be "only".
Here's a patch fixing those typos.
Attachments:
v1-0001-Fix-typos-from-LOCK-NOWAIT-improvement.patchapplication/octet-stream; name=v1-0001-Fix-typos-from-LOCK-NOWAIT-improvement.patchDownload
From b1183a287f882ba32ebeba3271f1eb682423c449 Mon Sep 17 00:00:00 2001
From: Will Mortensen <will@extrahop.com>
Date: Fri, 17 May 2024 23:32:47 -0700
Subject: [PATCH v1 1/1] Fix typos from LOCK NOWAIT improvement
(namely 2346df6fc373df9c5ab944eebecf7d3036d727de)
---
src/backend/storage/lmgr/lock.c | 2 +-
src/backend/storage/lmgr/proc.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 9e4ddf7225..f68c595c8a 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -1060,7 +1060,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
/*
* Check the proclock entry status. If dontWait = true, this is an
- * expected case; otherwise, it will open happen if something in the
+ * expected case; otherwise, it will only happen if something in the
* ipc communication doesn't work correctly.
*/
if (!(proclock->holdMask & LOCKBIT_ON(lockmode)))
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index a2900b6014..ce29da9012 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1054,8 +1054,8 @@ AuxiliaryPidGetProc(int pid)
* at exit.
*
* Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR
- * if not (if dontWait = true, this is a deadlock; if dontWait = false, we
- * would have had to wait).
+ * if not (if dontWait = true, we would have had to wait; if dontWait = false,
+ * this is a deadlock).
*
* ASSUME: that no one will fiddle with the queue until after
* we release the partition lock.
--
2.34.1
On 2024/5/18 14:38, Will Mortensen wrote:
On Tue, Mar 26, 2024 at 7:14 PM Will Mortensen <will@extrahop.com> wrote:
This comment on ProcSleep() seems to have the values of dontWait
backward (double negatives are tricky):* Result: PROC_WAIT_STATUS_OK if we acquired the lock,
PROC_WAIT_STATUS_ERROR
* if not (if dontWait = true, this is a deadlock; if dontWait = false, we
* would have had to wait).Also there's a minor typo in a comment in LockAcquireExtended():
* Check the proclock entry status. If dontWait = true, this is an
* expected case; otherwise, it will open happen if something in the
* ipc communication doesn't work correctly."open" should be "only".
Here's a patch fixing those typos.
Nice catch! The patch looks good to me.
--
Jingxian Li
On Fri, May 17, 2024 at 11:38:35PM -0700, Will Mortensen wrote:
On Tue, Mar 26, 2024 at 7:14 PM Will Mortensen <will@extrahop.com> wrote:
This comment on ProcSleep() seems to have the values of dontWait
backward (double negatives are tricky):* Result: PROC_WAIT_STATUS_OK if we acquired the lock,
PROC_WAIT_STATUS_ERROR
* if not (if dontWait = true, this is a deadlock; if dontWait = false, we
* would have had to wait).Also there's a minor typo in a comment in LockAcquireExtended():
* Check the proclock entry status. If dontWait = true, this is an
* expected case; otherwise, it will open happen if something in the
* ipc communication doesn't work correctly."open" should be "only".
Here's a patch fixing those typos.
Perhaps, this, err.. Should not have been named "dontWait" but
"doWait" ;)
Anyway, this goes way back in time and it is deep in the stack
(LockAcquireExtended, etc.) so it is too late to change: the patch
should be OK as it is.
--
Michael
On Sat, May 18, 2024 at 05:10:38PM +0800, Jingxian Li wrote:
Nice catch! The patch looks good to me.
And fixed that as well.
--
Michael