Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: add MVCCRangeKeyStack for range keys #84975

Merged

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Jul 24, 2022

storage: add MVCCRangeKeyStack for range keys

This patch adds MVCCRangeKeyStack and MVCCRangeKeyVersion, a new
range key representation that will be returned by SimpleMVCCIterator.
It is more compact, for efficiency, and comes with a set of convenience
methods to simplify common range key processing.

Resolves #83895.

Release note: None

storage: return MVCCRangeKeyStack from SimpleMVCCIterator

This patch changes SimpleMVCCIterator.RangeKeys() to return
MVCCRangeKeyStack instead of []MVCCRangeKeyValue. Callers have not
been migrated to properly make use of this -- instead, they call
AsRangeKeyValues() and construct and use the old data structure.

The MVCC range tombstones tech note is also updated to reflect this.

Release note: None

storage: migrate MVCC code to MVCCRangeKeyStack

Release note: None

*: migrate higher-level code to MVCCRangeKeyStack

Release note: None

kvserver/gc: partially migrate to MVCCRangeKeyStack

Some parts require invasive changes to MVCC stats helpers. These will
shortly be consolidated with other MVCC stats logic elsewhere, so the
existing logic is retained for now by using AsRangeKeyValues().

Release note: None

storage: remove FirstRangeKeyAbove() and HasRangeKeyBetween()

Release note: None

@erikgrinaker erikgrinaker self-assigned this Jul 24, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker
Copy link
Contributor Author

Would like some feedback on the type before I flesh this out and migrate all of the existing code. The motivation is reduced memory footprint and processing cost, a more ergonomic type, and common utility methods.

@erikgrinaker erikgrinaker requested a review from itsbilal July 24, 2022 20:32
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-data-structure branch 2 times, most recently from 552ed50 to e4f754f Compare July 25, 2022 09:21
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface makes a lot of sense to me.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, @itsbilal, and @nicktrav)


pkg/storage/mvcc_key.go line 462 at r1 (raw file):

func (s MVCCRangeKeyStack) Clone() MVCCRangeKeyStack {
	s.Bounds = s.Bounds.Clone()
	s.Versions = s.Versions.Clone()

Not sure how frequently we'll be cloning the entire stack, but to conserve allocations, this method could copy all the relevant slices into a single shared byte array since they'll all have the same lifetime.

@aliher1911
Copy link
Contributor

This looks similar to what I was doing in MVCCGarbageCollectRangeKeys to track range merges. So existing code there would be easy to adapt.

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-data-structure branch from e4f754f to f69215d Compare July 28, 2022 20:22
Copy link
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too. :shipit:

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 30 of 30 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, and @itsbilal)

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-data-structure branch 3 times, most recently from 39c5c81 to 2210add Compare July 29, 2022 15:24
@erikgrinaker erikgrinaker marked this pull request as ready for review July 29, 2022 15:27
@erikgrinaker erikgrinaker requested review from a team as code owners July 29, 2022 15:27
@erikgrinaker erikgrinaker requested review from a team and rhu713 July 29, 2022 15:27
Copy link
Contributor Author

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this should be good to go now.

Reviewed 1 of 32 files at r4, 11 of 30 files at r5, 5 of 6 files at r6, 8 of 8 files at r7, 6 of 6 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @itsbilal, @jbowens, and @rhu713)


pkg/storage/mvcc_key.go line 462 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Not sure how frequently we'll be cloning the entire stack, but to conserve allocations, this method could copy all the relevant slices into a single shared byte array since they'll all have the same lifetime.

Yeah, good idea. Added a TODO for this, but should be straightforward.

That said, we can avoid most of the cloning we do if cockroachdb/pebble#1775 can guarantee that it's safe to reuse the results of RangeKeys() as long as the range keys haven't changed. So let's do that first and then see if there's any cloning left in the hot paths -- hopefully there won't be.

@erikgrinaker erikgrinaker requested review from pav-kv and removed request for a team and rhu713 July 29, 2022 15:30
Copy link
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup. :lgtm:

Reviewed 32 of 32 files at r4, 30 of 30 files at r5, 6 of 6 files at r6, 8 of 8 files at r7, 6 of 6 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aliher1911, @itsbilal, @jbowens, and @pavelkalinnikov)

@erikgrinaker
Copy link
Contributor Author

Heads up @msbutler @gh-casper, we're making some minor changes to the data structure returned by MVCCIterator.RangeKeys(). Let me know if you need a hand with adapting your code.

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-data-structure branch from 2210add to f78f8e5 Compare July 29, 2022 16:04
This patch adds `MVCCRangeKeyStack` and `MVCCRangeKeyVersion`, a new
range key representation that will be returned by `SimpleMVCCIterator`.
It is more compact, for efficiency, and comes with a set of convenience
methods to simplify common range key processing.

Release note: None
This patch changes `SimpleMVCCIterator.RangeKeys()` to return
`MVCCRangeKeyStack` instead of `[]MVCCRangeKeyValue`. Callers have not
been migrated to properly make use of this -- instead, they call
`AsRangeKeyValues()` and construct and use the old data structure.

The MVCC range tombstones tech note is also updated to reflect this.

Release note: None
Some parts require invasive changes to MVCC stats helpers. These will
shortly be consolidated with other MVCC stats logic elsewhere, so the
existing logic is retained for now by using `AsRangeKeyValues()`.

Release note: None
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-data-structure branch from f78f8e5 to 4f78a76 Compare July 29, 2022 18:53
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 17 of 32 files at r4, 11 of 30 files at r11, 6 of 7 files at r12, 8 of 8 files at r13, 6 of 6 files at r14, 1 of 1 files at r15, 1 of 33 files at r16, 11 of 30 files at r17, 6 of 7 files at r18, 8 of 8 files at r19, 6 of 6 files at r20, 1 of 1 files at r21, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @aliher1911, @itsbilal, @jbowens, and @pavelkalinnikov)

@erikgrinaker
Copy link
Contributor Author

TFTRs! CI failures are known flake in TestRestoreOldVersions.

bors r=nicktrav,jbowens

@craig craig bot merged commit 7e2df69 into cockroachdb:master Jul 29, 2022
@craig
Copy link
Contributor

craig bot commented Jul 29, 2022

Build succeeded:

@erikgrinaker erikgrinaker deleted the mvcc-range-tombstones-data-structure branch August 1, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: new data type for MVCC range key stacks
5 participants