storage/spanset: clarify and clean up "reversed" span checks #45085
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
… bound This commit improves TestSpanSetBatchBoundaries and makes it test operations at the exclusive upper bound key of the declared span.
|
This change is |
ajwerner
approved these changes
Feb 13, 2020
There was a problem hiding this comment.
Thanks. This is much cleaner.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained
nvanbenschoten
force-pushed
the
nvanbenschoten/spanRevCheck
branch
2 times, most recently
from
32bafa3 to
9edb8f2
Compare
The notion of "reversed" span checks was introduced in 5176bac. That was a good change which allowed for proper validation of spans when using `spanset.Iterator.SeekLT`. However, the semantics around the `reversed` argument added to `SpanSet.checkAllowed` were strange and under-specified. Now that the start key of the span was exclusive, what did it mean to provide a reversed multi-key span to `checkAllowed`? Was the start key before or after the end key? Was it ok that only the exclusive portion of the span was being provided by all callers? Were reversed multi-key spans even supported? The comment said that "the reversed arguments makes the lower bound exclusive and the upper bound inclusive, i.e. [a,b) will be considered (a,b]". It's unclear whether this was mistakenly meaning that "[a,b) will be considered (b,a]". This all led to a terribly confusing condition: https://github.com/cockroachdb/cockroach/blob/5d69fd053ba52ae7ce94567b7b5fbb7cd857f1af/pkg/storage/spanset/spanset.go#L197. This commit clarifies these semantics by removing the `reversed` flag while retaining roughly the same idea in a way that's consistent with the existing meaning of a "Span". `SpanSet.checkAllowed` now supports an extended span format with a nil start key and a non-nil end key (e.g. "[nil, c)"). In this form, s2.Key (inclusive) is considered to be the previous key to s2.EndKey (exclusive). This avoids any ambiguity around multi-key "reversed" spans and fits in better with the existing definition of a Span.
nvanbenschoten
force-pushed
the
nvanbenschoten/spanRevCheck
branch
from
9edb8f2 to
aa13826
Compare
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Feb 14, 2020
45085: storage/spanset: clarify and clean up "reversed" span checks r=nvanbenschoten a=nvanbenschoten The notion of "reversed" span checks was introduced in 5176bac. That was a good change which allowed for proper validation of spans when using `spanset.Iterator.SeekLT`. However, the semantics around the `reversed` argument added to `SpanSet.checkAllowed` were strange and under-specified. Now that the start key of the span was exclusive, what did it mean to provide a reversed multi-key span to `checkAllowed`? Was the start key before or after the end key? Was it ok that only the exclusive portion of the span was being provided by all callers? Were reversed multi-key spans even supported? The comment said that "the reversed arguments makes the lower bound exclusive and the upper bound inclusive, i.e. [a,b) will be considered (a,b]". It's unclear whether this was mistakenly meaning that "[a,b) will be considered (b,a]". This all led to a terribly confusing condition: https://github.com/cockroachdb/cockroach/blob/5d69fd053ba52ae7ce94567b7b5fbb7cd857f1af/pkg/storage/spanset/spanset.go#L197. This commit clarifies these semantics by removing the `reversed` flag while retaining roughly the same idea in a way that's consistent with the existing meaning of a "Span". `SpanSet.checkAllowed` now supports an extended span format with a nil start key and a non-nil end key (e.g. "[nil, c)"). In this form, s2.Key (inclusive) is considered to be the previous key to s2.EndKey (exclusive). This avoids any ambiguity around multi-key "reversed" spans and fits in better with the existing definition of a Span. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Feb 20, 2020
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.
craig bot
pushed a commit
that referenced
this pull request
Feb 26, 2020
45232: storage/batcheval: declare intent resolution at txn MinTimestamp r=nvanbenschoten a=nvanbenschoten 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 #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. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
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.
The notion of "reversed" span checks was introduced in 5176bac. That was a good change which allowed for proper validation of spans when using
spanset.Iterator.SeekLT. However, the semantics around thereversedargument added toSpanSet.checkAllowedwere strange and under-specified. Now that the start key of the span was exclusive, what did it mean to provide a reversed multi-key span tocheckAllowed? Was the start key before or after the end key? Was it ok that only the exclusive portion of the span was being provided by all callers? Were reversed multi-key spans even supported? The comment said that "the reversed arguments makes the lower bound exclusive and the upper bound inclusive, i.e. [a,b) will be considered (a,b]". It's unclear whether this was mistakenly meaning that "[a,b) will be considered (b,a]". This all led to a terribly confusing condition:cockroach/pkg/storage/spanset/spanset.go
Line 197 in 5d69fd0
This commit clarifies these semantics by removing the
reversedflag while retaining roughly the same idea in a way that's consistent with the existing meaning of a "Span".SpanSet.checkAllowednow supports an extended span format with a nil start key and a non-nil end key (e.g. "[nil, c)"). In this form, s2.Key (inclusive) is considered to be the previous key to s2.EndKey (exclusive). This avoids any ambiguity around multi-key "reversed" spans and fits in better with the existing definition of a Span.