storage: use resume span to set ts cache#21304
storage: use resume span to set ts cache#21304spencerkimball merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
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 |
1e5d654 to
d9128ba
Compare
|
@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. |
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
d9128ba to
22f80e7
Compare
|
Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, all commit checks successful. Comments from Reviewable |
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 Reviewed 1 of 4 files at r1. Comments from Reviewable |
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:
and return resume span "c" - "d"
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