kv: don't consider lease start time as closed timestamp #62570
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/closedTSAfterMerge
branch
from
a98de4e to
dd97120
Compare
|
I just deflaked this test, so it should be stable now. |
andreimatei
approved these changes
Mar 30, 2021
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/closed_timestamp_test.go, line 431 at r1 (raw file):
// Update the batch to simulate a transaction that has a global uncertainty // limit after the lease transfer. Keep its read timestamp the same.
is there still a "lease transfer" around here?
Fixes cockroachdb#60929. Relates to cockroachdb#61986. Relates to cockroachdb#61989. This commit fixes a closed timestamp violation that could allow a value/intent write at a timestamp below a range's closed timestamp. This could allow for serializability violations if it allowed a follower read to miss a write and could lead to a panic in the rangefeed processor if a rangefeed was watching at the right time, as we saw in cockroachdb#60929. In cockroachdb#60929, we found that this bug was caused by a range merge and a lease transfer racing in such a way that the closed timestamp could later be violated by a write to the subsumed portion of the joint range. The root cause of this was an opportunistic optimization made in 7037b54 to consider a range's lease start time as an input to its closed timestamp computation. This optimization did not account for the possibility of serving writes to a newly subsumed keyspace below a range's lease start time if that keyspace was merged into a range under its current lease and with a freeze time below the current lease start time. This bug is fixed by removing the optimization, which was on its way out to allow for cockroachdb#61986 anyway. Note that removing this optimization does not break `TestClosedTimestampCanServeThroughoutLeaseTransfer`, because the v2 closed timestamp system does not allow for closed timestamp regressions, even across leaseholders. This was one of the many benefits of the new system.
nvanbenschoten
force-pushed
the
nvanbenschoten/closedTSAfterMerge
branch
from
dd97120 to
d77c3ed
Compare
nvanbenschoten
commented
Mar 30, 2021
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/kv/kvserver/closed_timestamp_test.go, line 431 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
is there still a "lease transfer" around here?
Done.
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #60929.
Relates to #61986.
Relates to #61989.
This commit fixes a closed timestamp violation that could allow a
value/intent write at a timestamp below a range's closed timestamp. This
could allow for serializability violations if it allowed a follower read
to miss a write and could lead to a panic in the rangefeed processor if
a rangefeed was watching at the right time, as we saw in #60929.
In #60929, we found that this bug was caused by a range merge and a
lease transfer racing in such a way that the closed timestamp could
later be violated by a write to the subsumed portion of the joint range.
The root cause of this was an opportunistic optimization made in 7037b54
to consider a range's lease start time as an input to its closed
timestamp computation. This optimization did not account for the
possibility of serving writes to a newly subsumed keyspace below a
range's lease start time if that keyspace was merged into a range under
its current lease and with a freeze time below the current lease start
time. This bug is fixed by removing the optimization, which was on its
way out to allow for #61986 anyway.
Note that removing this optimization does not break
TestClosedTimestampCanServeThroughoutLeaseTransfer, because the v2closed timestamp system does not allow for closed timestamp regressions,
even across leaseholders. This was one of the many benefits of the new
system.