storage: use resume span to set ts cache

[spencerkimball]

Previously, we were updating the timestamp cache based on the rows
returned during scans and reverse scans. However, not only was this
inconsistent with how we gather intent spans for resolving on end
transaction, it was actually out of step with results actually
read, meaning we were not setting the timestamp cache properly to
reflect results read and returned. Consider the following scenario:

• Scan from "a" to "d", max results stops at "b".
• Current MVCCScan will seek past "b", find next key (let's say "c")
  and return resume span "c" - "d"
• However, we were updating timestamp cache for the span "a" - "b".Next().
• That leaves all of the keys from "b".Next() - "c" unprotected by
  the timestamp cache, so history can be rewritten under the span
  on successive resumes at the same timestamp.

Now, we correctly use the resume span to update the timestamp cache,
which is both consistent with other uses and correctly updates the
timestamp cache based on results actually read.

Release note: None

[cockroach-teamcity]

This change is 

[petermattis]

We were previously always seeking to the next key beyond the maximum
keys allowed for a scan in order to find out if there were any more
left in the iteration. However, that case is the exception, not the
rule and the performance is impacted for any iteration which sets
max keys to 1.

How many iterations set max-keys to 1? I don't think SQL queries do that, but perhaps they do. Have you checked?

---

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[spencerkimball]

@petermattis actually I backed out that part of the change. It broke some really nasty tests and I think it's safer just not to change behavior, but to fix the timestamp cache issues only. PTAL.

[petermattis]

---

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[nvanbenschoten]

We were previously always seeking to the next key beyond the maximum
keys allowed for a scan in order to find out if there were any more
left in the iteration. However, that case is the exception, not the
rule and the performance is impacted for any iteration which sets
max keys to 1.

I'm curious what the change that was backed out was doing. Just so we have a documented history, do you mind explaining?

With your comment in mind, it looks to me like resumeSpans may have been defined questionably in the first place. I would expect that we would return lastKey.Next() as the start of a forward resume span, or even just lastKey if we added an "exclusive start" flag on ScanRequests. This way, we would never miss an update to the tscache because any subsequent scans would start immediately after the previous scan stopped, with no gaps in-between. Was adjusting the resumeSpan semantics the change you tried to make?

---

Reviewed 1 of 4 files at r1.
Review status: 1 of 4 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable