storage/batcheval: declare intent resolution at txn MinTimestamp #45232
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 simplifies a number of callers.
This prevents the hazard described in https://github.com/cockroachdb/cockroach/blob/5f63ac527becd4aae5cfbdaa76b7de28e07b8767/pkg/storage/concurrency/concurrency_control.go#L480. I've been trying to (starting with cockroachdb#45085) clean up `spanset.Batch` to the point where it would have been able to detect this unlatched key access, but getting that all the way over the fence is a little tricky due to: - `GCRequest` span declaration - should this even latch? - transactional `Put` span declaration - does this need to declare a write span all the way back to txn.MinTimestamp because it might move an existing intent forward? - `spanset.Iterator` semantics and its interaction with `pebbleMVCCScanner` - what can the `spanset.Iterator` even assert here, given that the scanner itself is determining whether to ignore values or not. Unfortunately, without a rework, the current attempt at asserting correct timestamp access in `spanset.Batch` is hopelessly broken. Not only does the verification not encode the correct rules for declared timestamps (e.g. a write at time 10 should permit writing at any time >= 10), but the timestamp it works with isn't even the correct timestamp. It compares the declared span timestamps against the batch header timestamp, which completely misses the point. It should be comparing the declared span timestamps against the timestamps of actual uses of the `spanset.Batch` so that we're actually asserting that the batch is being used correctly. I'd like to fix all of this, but not here.
|
This change is |
35 tasks
ajwerner
approved these changes
Feb 26, 2020
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 3 of 3 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained
|
bors r+ |
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.
This prevents the hazard described in:
cockroach/pkg/storage/concurrency/concurrency_control.go
Line 480 in 5f63ac5
I've been trying to (starting with #45085) clean up
spanset.Batchto the point where it would have been able to detect this unlatched key access, but getting that all the way over the fence is a little tricky due to:GCRequestspan declaration - should this even latch?Putspan declaration - does this need to declare a write span all the way back to txn.MinTimestamp because it might move an existing intent forward?spanset.Iteratorsemantics and its interaction withpebbleMVCCScanner- what can thespanset.Iteratoreven assert here, given that the scanner itself is determining whether to ignore values or not.Unfortunately, without a rework, the current attempt at asserting correct timestamp access in
spanset.Batchis hopelessly broken. Not only does the verification not encode the correct rules for declared timestamps (e.g. a write at time 10 should permit writing at any time >= 10), but the timestamp it works with isn't even the correct timestamp. It compares the declared span timestamps against the batch header timestamp, which completely misses the point. It should be comparing the declared span timestamps against the timestamps of actual uses of thespanset.Batchso that we're actually asserting that the batch is being used correctly.I'd like to fix all of this, but not here.