Reset waitStart when a lock wait fails

On Sep 24, 2026, at 09:30, shihao zhong <zhong950419@gmail.com> wrote:

Hi,

Alex and Sami noticed in [1] that RemoveFromWaitQueue() does not clear
PGPROC->waitStart. After lock_timeout, cancel or deadlock the old value
stays until the next lock wait.

It leaks into pg_locks for a short time. The next wait joins the queue
before ProcSleep() stores the new waitStart, and in that window pg_locks
shows the old start time instead of NULL. Backend stopped at ProcSleep()
entry, a few seconds after a lock_timeout:

pid | relation | mode | granted | waitstart
-------+----------+---------------------+---------+-------------------------------
42442 | t | AccessExclusiveLock | f | 2026-09-23 21:01:39.274763-04

With the patch it reads NULL. The patch clears it the same way
ProcWakeup() does, like 70f470314cb did for the grant path. It applies
to 14 and up.

[1] /messages/by-id/CAPrb+Q+XN=sNusXiUeWmMo2H7Qgq3Y4uPekSSLkHcnCyf7GhXg@mail.gmail.com

Thanks,
Shihao
<v1-0001-Reset-waitStart-when-a-lock-wait-fails.patch>

Looks good to me. Once proc->waitLock is cleared, proc->waitStart no longer describes an active wait, so clearing it at the same time seems right.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, Sep 24, 2026 at 10:37:12AM +0800, Chao Li wrote:

On Sep 24, 2026, at 09:30, shihao zhong <zhong950419@gmail.com> wrote:

With the patch it reads NULL. The patch clears it the same way
ProcWakeup() does, like 70f470314cb did for the grant path. It applies
to 14 and up.

Looks good to me. Once proc->waitLock is cleared, proc->waitStart no
longer describes an active wait, so clearing it at the same time
seems right.

Yep, it looks weird to not clean up the field in this code path.
@Fujii-san, what do you think?
--
Michael

On Thu, Sep 24, 2026 at 2:09 PM Michael Paquier <michael@paquier.xyz> wrote:

Looks good to me. Once proc->waitLock is cleared, proc->waitStart no
longer describes an active wait, so clearing it at the same time
seems right.

Yep, it looks weird to not clean up the field in this code path.
@Fujii-san, what do you think?

The patch looks good to me!

BTW, ProcWakeup() can reset waitStart before the waiter updates it in
ProcSleep() (or ResolveRecoveryConflictWithLock() during hot standby)?
If so, seems waitStart could remain set even after the wait, causing the same
issue. To fix this, we should reset waitStart at the end of ProcSleep() and
in LockErrorCleanup() as well?

Regards,

--
Fujii Masao

BTW, ProcWakeup() can reset waitStart before the waiter updates it in
ProcSleep() (or ResolveRecoveryConflictWithLock() during hot standby)?
If so, seems waitStart could remain set even after the wait, causing the

same

issue. To fix this, we should reset waitStart at the end of ProcSleep()

and

in LockErrorCleanup() as well?

Yes, it happens. I held the waiter at ProcSleep() entry while the holder
committed, and waitStart stayed set after the wait.

v2 attached. 0001 is unchanged. 0002 clears waitStart at the end of
ProcSleep() and in LockErrorCleanup(). The LockErrorCleanup() part is for
a cancel that arrives after the grant, where ProcSleep() errors out before
its end. With 0002 the standby shows the start of the current wait, and
the other cases read NULL.

On 14 to 17, 0002 needs a small context fix
since ResetAwaitedLock() is "lockAwaited = NULL" there.

Thanks,
Shihao

Hi,

v2 attached. 0001 is unchanged. 0002 clears waitStart at the end of
ProcSleep() and in LockErrorCleanup(). The LockErrorCleanup() part is
for a cancel that arrives after the grant, where ProcSleep() errors
out before its end. With 0002 the standby shows the start of the
current wait, and the other cases read NULL.

I reviewed v2 (0001 and 0002). I applied it on master (1a846a555a)
and built it on macOS with assertions and injection points enabled.
The build has no warnings. These tests pass: regress, isolation,
test_misc/011_lock_stats, recovery/031_recovery_conflict,
recovery/027_stream_regress and injection_points/isolation.

I added local injection points just before waitStart is set, in
ProcSleep() and in ResolveRecoveryConflictWithLock(), and one just
after it in ProcSleep().

After each case, I started another lock wait and paused the backend
before it set waitStart. At that point, pg_locks.waitstart should be
NULL. Then it starts a new wait, and I stop it before it sets
waitStart. At this point pg_locks.waitstart should be NULL.

On HEAD, it shows the start time of the previous wait in all these
cases: after lock_timeout, after a cancel, after a deadlock, after a
grant that happens before waitStart is set, and after such a grant
followed by a cancel before the end of ProcSleep().

With 0001 only, the first three cases show NULL, but the last two
still show the old time.

With 0002 only, and with v2, all cases show NULL.

On a standby, the startup process shows the old time during its
whole next wait on HEAD and with 0001 only. With 0002 only and with
v2, it shows the start time of the current wait.

A normal wait that ends with a grant shows NULL in all builds.

A few comments:

1) I think 0001 is not needed anymore once 0002 is applied. With
0002, the waiting backend always clears waitStart itself when the wait
ends at the end of ProcSleep(), or in LockErrorCleanup().
RemoveFromWaitQueue() is only called from CheckDeadLock() and
LockErrorCleanup(), and both paths reach one of these new resets.

Maybe it is simpler to merge 0001 and 0002 into one commit?
They fix the same problem and would be backpatched together.

2) 0001 has "Backpatch-through: 14", but 0002 has no such line. The
race fixed by 0002 exists in 14-17 too: there, ProcSleep() also
releases the partition lock before it sets waitStart. Only the
context of the LockErrorCleanup() hunk is different in 14-17. With that
change, both hunks apply to REL_14_STABLE and REL_17_STABLE.

Best regards,
Andrew Krylosov

Hi Andrew,

1) I think 0001 is not needed anymore once 0002 is applied. With
0002, the waiting backend always clears waitStart itself when the wait
ends at the end of ProcSleep(), or in LockErrorCleanup().
RemoveFromWaitQueue() is only called from CheckDeadLock() and
LockErrorCleanup(), and both paths reach one of these new resets.

Maybe it is simpler to merge 0001 and 0002 into one commit?
They fix the same problem and would be backpatched together.

Agreed, v3 merges them into one patch. The code is the same as v2.

You are right that the RemoveFromWaitQueue() line is no longer needed.
I kept it so it matches ProcWakeup(), since Chao and Michael wanted the
field cleared there. I'm fine with dropping it if Fujii-san prefers. On
14 to 17 there is a third caller, the early deadlock exit in
ProcSleep(), but it runs before waitStart is set, so your point holds
there too.

2) 0001 has "Backpatch-through: 14", but 0002 has no such line.

My mistake. v3 has Backpatch-through: 14.

Thanks,
Shihao

On Mon, Sep 28, 2026 at 12:51 AM shihao zhong <zhong950419@gmail.com> wrote:

Hi Andrew,

1) I think 0001 is not needed anymore once 0002 is applied. With
0002, the waiting backend always clears waitStart itself when the wait
ends at the end of ProcSleep(), or in LockErrorCleanup().
RemoveFromWaitQueue() is only called from CheckDeadLock() and
LockErrorCleanup(), and both paths reach one of these new resets.

Maybe it is simpler to merge 0001 and 0002 into one commit?
They fix the same problem and would be backpatched together.

Agreed, v3 merges them into one patch. The code is the same as v2.

Thanks for updating the patch! I made only cosmetic changes and pushed it.

Regards,

--
Fujii Masao

Hi Fujii,

Thanks for commit this patch.

Thanks,
Shihao