storage: use new range tombstone stack for gc ops #86742
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 |
erikgrinaker
approved these changes
Aug 24, 2022
pkg/storage/mvcc.go
Outdated
| rangeTombstoneTss = rangeTombstoneTss[:0] | ||
| for _, v := range rangeKeys.Versions { | ||
| rangeTombstoneTss = append(rangeTombstoneTss, v.Timestamp) | ||
| // We can rely on range key changed iterator functionality here as we do |
There was a problem hiding this comment.
Suggested change
| // We can rely on range key changed iterator functionality here as we do | |
| // We can't rely on range key changed iterator functionality here as we do |
There was a problem hiding this comment.
RangeKeyChanged() does fire correctly on seek operations too, but one has to take care to track it for every positioning operation. I'm fine with not doing it now though, this is simple and more obviously correct.
There was a problem hiding this comment.
I think tracking for all Next() is a non-starter here for now. We rely on get metadata to seek and potentially Next() as a side effect +all the subsequent iterations. We'll have to hasChanged |= iter.RangeKeyChaneged() through the whole method till it reaches this point. That would be too fragile even if we know that key doesn't change while iterating key's versions.
This commit replaces bespoke range tombstone stack manipulation logic with functions provided by MVCCRangeKeyStack type. Release justification: code cleanup not changing functionality. Release note: None
aliher1911
force-pushed
the
gc_iter_use_new_iter
branch
from
9460e0f to
40962cf
Compare
erikgrinaker
approved these changes
Aug 24, 2022
|
bors r+ |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
Build succeeded: |
DrewKimball
added a commit
to DrewKimball/cockroach
that referenced
this pull request
Sep 8, 2022
This commit modifies the sqlsmith logic to add all columns to the `OVER` clause for all window functions except for the ranking functions. This is necessary because all other window/aggregate functions can produce different results for different rows within a peer group. Ordering on all columns ensures that we are ordering on a key, which restricts each peer group to one row (and therefore ensures there is only one possible order within each peer group). This commit also reverts cockroachdb#86742, since window functions should now be deterministic. Fixes cockroachdb#87353 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Sep 13, 2022
87591: roachtest: make window functions deterministic r=DrewKimball a=DrewKimball This commit modifies the sqlsmith logic to add all columns to the `OVER` clause for all window functions except for the ranking functions. This is necessary because all other window/aggregate functions can produce different results for different rows within a peer group. Ordering on all columns ensures that we are ordering on a key, which restricts each peer group to one row (and therefore ensures there is only one possible order within each peer group). This commit also reverts #86742, since window functions should now be deterministic. Fixes #87353 Release note: None Co-authored-by: DrewKimball <[email protected]>
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 commit replaces bespoke range tombstone stack manipulation
logic with functions provided by MVCCRangeKeyStack type.
Release justification: code cleanup not changing functionality.
Release note: None