storage/concurrency: release reqs from same txn as discovered lock

[nvanbenschoten]

This commit updates the AddDiscoveredLock state transition in lockTableImpl to release all waiting writers that are part of the same transaction as a discovered lock.

This caused issues in #45482, where we saw txn deadlocks in the pkg/sql/tests.Bank benchmark. This issue was triggered because we were forgetting to inform the lockTable of a lock acquisition in a subtle case. That is now fixed and it's not clear that we can actually hit the bug here anymore given the current policy on how wait-queues form in the lock-table. Regardless, this is worth handling correctly in case things ever change.

[cockroach-teamcity]

This change is 

[sumeerbhola]

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/storage/concurrency/lock_table.go, line 1211 at r1 (raw file):

	// If there are waiting requests from the same txn, they no longer need to wait.
	if l.releaseWritersFromTxn(txn) {
		informWaiters = true

I think we can tighten up the semantics of informWaiters and remove the return value from releaseWritersFromTxn.
The only correctness need for informWaiters=false is when the lock is already held. Currently the code also sets it to false when l.reservation == nil but that is an premature performance optimization that I think we should remove.

[nvanbenschoten]

TFTR!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

pkg/storage/concurrency/lock_table.go, line 1211 at r1 (raw file):

Previously, sumeerbhola wrote…

I think we can tighten up the semantics of informWaiters and remove the return value from releaseWritersFromTxn.
The only correctness need for informWaiters=false is when the lock is already held. Currently the code also sets it to false when l.reservation == nil but that is an premature performance optimization that I think we should remove.

I'm not sure I understand why it's ever incorrect to call informActiveWaiters. You said that there's a correctness need for informWaiters=false when the lock is already held. Could you expand on that bit more?

Also, in place of the return value on releaseWritersFromTxn, are you suggesting that it call tryMakeNewDistinguished directly when it detects that doing so it necessary.

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/storage/concurrency/lock_table.go, line 1211 at r1 (raw file):

I'm not sure I understand why it's ever incorrect to call informActiveWaiters. You said that there's a correctness need for informWaiters=false when the lock is already held. Could you expand on that bit more?

I agree that from a correctness perspective it is ok to call informActiveWaiters even if nothing has changed. The use of informWaiters is trying to be a performance optimization. When l.reservation==nil and the lock was not held this performance optimization is limited to not calling informActiveWaiters since that function won't have any work to do -- there can't be any waiters. So we can remove it. Regarding setting informWaiters to false when the lock was already held -- now that there are more fields in waitingState, I don't think that optimization is correct since the waiters want to know whether they are waiting on a replicated or unreplicated lock. So that would mean removing informWaiters altogether.

Also, in place of the return value on releaseWritersFromTxn, are you suggesting that it call tryMakeNewDistinguished directly when it detects that doing so it necessary.

No, we'd still call informActiveWaiters here. I should have said that the return value from releaseWritersFromTxn should not be needed to decide whether to call informActiveWaiters.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)

---

pkg/storage/concurrency/lock_table.go, line 1211 at r1 (raw file):

Previously, sumeerbhola wrote…

I'm not sure I understand why it's ever incorrect to call informActiveWaiters. You said that there's a correctness need for informWaiters=false when the lock is already held. Could you expand on that bit more?

I agree that from a correctness perspective it is ok to call informActiveWaiters even if nothing has changed. The use of informWaiters is trying to be a performance optimization. When l.reservation==nil and the lock was not held this performance optimization is limited to not calling informActiveWaiters since that function won't have any work to do -- there can't be any waiters. So we can remove it. Regarding setting informWaiters to false when the lock was already held -- now that there are more fields in waitingState, I don't think that optimization is correct since the waiters want to know whether they are waiting on a replicated or unreplicated lock. So that would mean removing informWaiters altogether.

Also, in place of the return value on releaseWritersFromTxn, are you suggesting that it call tryMakeNewDistinguished directly when it detects that doing so it necessary.

No, we'd still call informActiveWaiters here. I should have said that the return value from releaseWritersFromTxn should not be needed to decide whether to call informActiveWaiters.

Done, I think. Mind double checking that that's the change you were thinking of?

[sumeerbhola]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

[nvanbenschoten]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[nvanbenschoten]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)