kvserver: serve reads on outgoing leaseholder and improve lease transfer latching

[irfansharif]

Is your feature request related to a problem? Please describe.

Consider the following trace (from an internal escalation: https://github.com/cockroachlabs/support/issues/1533):

‹     1.215ms      0.017ms                    event:kv/kvserver/spanlatch/manager.go:517 [n250,s576,r625905/2:/Table/62/16/1498341{40…-63…}] waiting to acquire read latch /Table/62/16/14983414806{3-4}@1651156511.195997554,0, held by write latch /M{in-ax}@0,0›
‹  1774.996ms   1773.781ms                    event:kv/kvserver/concurrency/concurrency_manager.go:300 [n250,s576,r625905/2:/Table/62/16/1498341{40…-63…}] scanning lock table for conflicting locks›
‹  1775.971ms      0.975ms                    event:kv/kvserver/replica_follower_read.go:94 [n250,s576,r625905/2:/Table/62/16/1498341{40…-63…}] can't serve follower read; closed timestamp too low by: 2m1.558486052s; maxClosed: 1651156389.637511502,0 ts: 1651156510.695997554,0 uncertaintyLimit: 1651156511.195997554,0›
‹  1777.982ms      2.011ms                    event:kv/kvserver/replica_send.go:120 [n250,s576,r625905/2:/Table/62/16/1498341{40…-63…}] replica.Send got error: [NotLeaseHolderError] lease held by different store; r625905: replica (n250,s576):2 not lease holder; current lease is repl=(n225,s477):6 seq=27689 start=1651156512.469961984,0 epo=3 pro=1651156509.637904941,0›

What we're seeing is a read wanting to grab a read latch at timestamp 1651156511.195997554,0, the lease is transferred away, with start time of 1651156512.469961984,0. We could've served the read at the now-old leaseholder, but chose not to do so. When we load-balance leases away from nodes running hot (what we're seeing above), lease transfers can be more disruptive than they need to be WRT the tail -- here we'd bubble the error all the way back up to the distsender and retry at the new leaseholder, incurring an additional RTT that shows up prominently in the tail (especially if the range is experiencing a large volume of traffic).

Describe the solution you'd like

Serving these reads when safe to do so. While here, we could also not acquire a non-MVCC write latch over the entire keyspan when evaluating a lease transfer (instead grabbing a write latch at the start time of the new lease). The non-MVCC write latch can be disruptive: the FIFO queuing you get for latch acquisitions + the non-MVCC latch declaration lease transfers (common during periods of high load) add to give us the property that concurrency over that keyspace will slowly drop down 1 until the lease transfer is evaluated, and then back up to what it was before. This is also bad from a latency perspective — requests that were serialized after the lease transfer have to wait for the last of the requests serialized before it to have finished, the lease transfer itself, before it’s able to start evaluation. We've noted this possibility here:

cockroach/pkg/kv/kvserver/batcheval/cmd_lease_transfer.go

Lines 46 to 55 in 609825e

	// We could, in principle, declare these latches as MVCC writes at the time
	// of the new lease. Doing so would block all concurrent writes but would
	// allow reads below the new lease timestamp through. However, doing so
	// would only be safe if we also accounted for clock uncertainty in all read
	// latches so that any read that may need to observe state on the new
	// leaseholder gets blocked. We actually already do this for transactional
	// reads (see DefaultDeclareIsolatedKeys), but not for non-transactional
	// reads. We'd need to be careful here, so we should only pull on this if we
	// decide that doing so is important.
	declareAllKeys(latchSpans)

As of d676d7e we've started giving non-transactional reads uncertainty timestamps, so we could improve our latching story now. This would not only prevent lease transfers from blocking earlier reads, it would also prevent earlier (potentially long-running) reads from blocking lease transfers.

Additional Context

Aside: we have a similar problem to the non-MVCC lease acquisitions for lease transfers but for split requests: #22348. The effect on observable latencies and queueing behaviour apply similarly.

+cc @cockroachdb/kv-notifications.

Jira issue: CRDB-15115

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!