storage: evaluate limited scans optimistically without latching

[nvanbenschoten]

Fixes #9521.
Supersedes #31904.

SQL has a tendency to create scans which cover a range's entire key span, though looking only to return a finite number of results. These requests end up blocking on writes that are holding latches over keys that the scan will not actually touch. In reality, when there is a scan with a limit, the actual affected key span ends up being a small subset of the range's key span.

This change creates a new "optimistic evaluation" path for read-only requests. When evaluating optimistically, the batch will sequence itself with the latch manager, but will not wait to acquire all of its latches. Instead, it begins evaluating immediately and verifies that it would not have needed to wait on any latch acquisitions after-the-fact. When performing this verification, it uses knowledge about the limited set of keys that the scan actually looked at. If there are no conflicts, the scan succeeds. If there are conflicts, the request waits for all of its latch acquisition attempts to finish and re-evaluates.

This PR replaces #31904. The major difference between the two is that this PR exploits the structure of the latch manager to efficiently perform optimistic latch acquisition and after-the-fact verification of conflicts. Doing this requires keeping no extra state because it uses the immutable snapshots that the latch manager now captures during sequencing. The other major difference is that this PR does not release latches after a failed optimistic evaluation.

NOTE: a prevalent theory about the pathological case addressed in this PR was that overestimated read latches would serialize with write latches and write latches would serialize with overestimated read latches, causing all requests on a range to serialize. I wasn't seeing this "chaining" occur in practice. It turns out that the "timestamp awareness" in the latch manager should avoid this behavior in most cases because later writes will have higher timestamps than earlier reads, so they won't wait on them to execute. An argument could be made that because the hypothesized serialization of requests doesn't actually exist, the added complexity of this change isn't justified. I'm curious what reviewers think.

Benchmark Results

name                                   old ops/sec  new ops/sec  delta
kv95/cores=16/nodes=3/splits=3          51.9k ± 0%   51.7k ± 1%     ~     (p=0.400 n=3+3)
kvS70-L1/cores=16/nodes=3/splits=3      24.1k ± 4%   27.7k ± 1%  +14.75%  (p=0.100 n=3+3)
kvS70-L5/cores=16/nodes=3/splits=3      24.5k ± 1%   27.5k ± 1%  +12.08%  (p=0.100 n=3+3)
kvS70-L1000/cores=16/nodes=3/splits=3   16.0k ± 1%   16.6k ± 2%   +3.79%  (p=0.100 n=3+3)

name                                   old p50(ms)  new p50(ms)  delta
kv95/cores=16/nodes=3/splits=3           0.70 ± 0%    0.70 ± 0%     ~     (all equal)
kvS70-L1/cores=16/nodes=3/splits=3       1.07 ± 6%    0.90 ± 0%  -15.62%  (p=0.100 n=3+3)
kvS70-L5/cores=16/nodes=3/splits=3       1.10 ± 0%    0.90 ± 0%  -18.18%  (p=0.100 n=3+3)
kvS70-L1000/cores=16/nodes=3/splits=3    1.80 ± 0%    1.67 ± 4%   -7.41%  (p=0.100 n=3+3)

name                                   old p99(ms)  new p99(ms)  delta
kv95/cores=16/nodes=3/splits=3           1.80 ± 0%    1.80 ± 0%     ~     (all equal)
kvS70-L1/cores=16/nodes=3/splits=3       5.77 ±32%    4.70 ± 0%     ~     (p=0.400 n=3+3)
kvS70-L5/cores=16/nodes=3/splits=3       5.00 ± 0%    4.70 ± 0%   -6.00%  (p=0.100 n=3+3)
kvS70-L1000/cores=16/nodes=3/splits=3    6.90 ± 3%    7.33 ± 8%     ~     (p=0.400 n=3+3)

S<num> = --span-percent=<num>, L<num> = --span-limit=<num>

Release note (performance improvement): improved performance on workloads which mix OLAP queries with inserts and updates.

[cockroach-teamcity]

This change is 

[tbg]

I have to review the main commit more closely, but on the face of it the complexity seems manageable. Whether this should be merged or not depends on the impact it would have on real-world workloads. Intuitively the change seems to make a lot of sense to me, though I don't know all the situations in which suitable scans are emitted by the SQL layer. My expectation is that we'll see them during DistSQL scans (with a relatively large limit), though in that case the scans would eventually scan all of the range and so would likely conflict with any writes each individual scan would "avoid". OLAP mixed with updates seemed to have been the original motivator in #31904, though no data was provided to back this up.

Does this change move the needle on TPCC at all? What about TPCH (or are we in no position to even consider that?)

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 5 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/replica.go, line 2284 at r4 (raw file):

				resp := br.Responses[i].GetInner().(*roachpb.ScanResponse)
				if resp.ResumeSpan != nil {
					if start.Equal(resp.ResumeSpan.Key) {

Annoying that there's no "better" way to catch this. Can you add a unit test, though?

[petermattis]

Does this change move the needle on TPCC at all? What about TPCH (or are we in no position to even consider that?)

TPCH is read-only, so this change shouldn't affect it. What it could affect is running TPCC and then periodically issuing an OLAP-style query? I believe a workload like that is what motivated this change.

Reviewable status: complete! 0 of 0 LGTMs obtained

[nvanbenschoten]

My expectation is that we'll see them during DistSQL scans (with a relatively large limit), though in that case the scans would eventually scan all of the range and so would likely conflict with any writes each individual scan would "avoid".

Yeah, this is what inspired the "if the limit is smaller than the number of live keys in the range" check.

I ran a tpccbench run with this change and one without it and ended up getting the exact same warehouse count (1535). I'll run a few more and then move on to other workloads like Peter's TPC-C + periodic TPC-H suggestion.

[nvanbenschoten]

Closing in favor of #66059, which implements this change in the framework of the concurrency manager.