storage/concurrency: don't push reservation transactions

[nvanbenschoten]

This commit updates the lockTableWaiter to no longer push reservation transactions. The waiter now only pushes the holders of conflicting locks that are held by other transactions.

This is because a transaction push will block until the pushee transaction has either committed or aborted. It is therefore not appropriate for cases where there is still a chance for the pusher to make progress without the pushee completing first. This is not true when the waiter is waiting on a held lock - the lock must be released before the waiter may proceed. However, it is true when the waiter is waiting on a reservation - the reservation holder may release the reservation without acquiring a lock, allowing the waiter to proceed without conflict.

This was the cause of deadlocks in some of the experiments I was running. The exact sequence I was running into was that:

1. key K had a lock wait-queue
2. txn A acquired a reservation for key K
3. txn B queued behind txn A, becoming the distinguished waiter
4. txn B began pushing txn A
5. txn A hit a WriteTooOld error and did not acquire a lock
6. txn A exited the lock wait-queue
7. txn A handled the WriteTooOld error, returned to key K, and queued behind txn B
8. txn B waited on txn A indefinitely in the txnWaitQueue
9. deadlock!

[cockroach-teamcity]

This change is 

[sumeerbhola]

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

---

pkg/storage/concurrency/lock_table_waiter.go, line 88 at r2 (raw file):

				// conflict.
				// NOTE: we could pull this logic into pushTxn, but then we'd be
				// creating useless timers.

The "therefore not appropriate" sentence followed by the "This is not true" sentence were a bit hard to read, so I tried to rephrase in a simpler form below, and added some further detail, in case you can use that to adjust the comment.

// If the lock is not held and instead has a reservation, we don't
// want to push the reservation transaction. A transaction push will
// block until the pushee transaction has either committed or
// aborted or pushed or rolled back savepoints, i.e., there is some state
// change that has happened to the transaction record that unblocks the
// pusher. It will not unblock merely because a request issued by the
// pushee transaction has completed and released a reservation. Note
// that:
// - reservations are not a guarantee that the lock will be acquired.
// - the two reasons to push do not apply to requests holding reservations:
// - competing requests compete at exactly one lock table, so there is no
// possibility of distributed deadlock due to reservations.
// - the lock table can prioritize requests based on transaction priorities (TODO(sbhola) ...)

---

pkg/storage/concurrency/lock_table_waiter.go, line 96 at r2 (raw file):

				// reservations?
				// TODO(sbhola): does this allow us to get rid of the concept of a
				// reservaton entirely? Is there a reason to keep it?

it is likely that we can eliminate reservations from the lock table.

[nvanbenschoten]

bors r+

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

---

pkg/storage/concurrency/lock_table_waiter.go, line 88 at r2 (raw file):

Previously, sumeerbhola wrote…

The "therefore not appropriate" sentence followed by the "This is not true" sentence were a bit hard to read, so I tried to rephrase in a simpler form below, and added some further detail, in case you can use that to adjust the comment.

// If the lock is not held and instead has a reservation, we don't
// want to push the reservation transaction. A transaction push will
// block until the pushee transaction has either committed or
// aborted or pushed or rolled back savepoints, i.e., there is some state
// change that has happened to the transaction record that unblocks the
// pusher. It will not unblock merely because a request issued by the
// pushee transaction has completed and released a reservation. Note
// that:
// - reservations are not a guarantee that the lock will be acquired.
// - the two reasons to push do not apply to requests holding reservations:
// - competing requests compete at exactly one lock table, so there is no
// possibility of distributed deadlock due to reservations.
// - the lock table can prioritize requests based on transaction priorities (TODO(sbhola) ...)

Thanks for the suggestion. Done.

---

pkg/storage/concurrency/lock_table_waiter.go, line 96 at r2 (raw file):

Previously, sumeerbhola wrote…

it is likely that we can eliminate reservations from the lock table.

Ack. That will be a nice simplification.

[craig]

Build succeeded

• GitHub CI (Cockroach)