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
