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

batcheval: add MVCC-compliant RevertRange variant #76478

Merged
merged 3 commits into from
Feb 22, 2022

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Feb 13, 2022

roachpb: add isAlone for RevertRangeRequest

Since RevertRange mutates MVCC history, we want them to be alone in a
batch. The DistSender will split any batches that have multiple such
requests.

Release note: None

storage: add NextKeyIgnoringTime() for MVCCIncrementalIterator

This patch adds a method NextKeyIgnoringTime() for
MVCCIncrementalIterator. This can be used to find the next key (as
opposed to version) of the iterator, ignoring the time bounds. It's
similar to NextIgnoringTime(), but calls NextKey() instead of
Next() on the underlying iterator.

Release note: None

batcheval: add MVCC-compliant RevertRange variant

This adds a new parameter ExperimentalPreserveHistory which, rather
than clearing keys above the target time, will write new values or
tombstones that reflect the state at the target time. For long runs of
new keys, this will instead drop an MVCC range tombstone. This makes the
command respect e.g. MVCC immutability, the closed timestamp, and
timestamp cache.

Note that MVCC range tombstones are currently experimental, and as such
this parameter is also experimental. Callers must call
storage.CanUseExperimentalMVCCRangeTombstones() before using it.

Resolves #70416.

Release note: None

@erikgrinaker erikgrinaker self-assigned this Feb 13, 2022
@erikgrinaker erikgrinaker requested review from a team as code owners February 13, 2022 16:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

Just one API question from me

pkg/roachpb/api.go Outdated Show resolved Hide resolved
pkg/roachpb/api.proto Show resolved Hide resolved
pkg/storage/mvcc_incremental_iterator.go Show resolved Hide resolved
Copy link
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

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

Mostly nits about comments, but I think there's some room for improvement in ExperimentalMVCCRevertRange which we should address before in becomes non-experimental.

pkg/storage/mvcc_incremental_iterator.go Show resolved Hide resolved
pkg/roachpb/api.go Outdated Show resolved Hide resolved
pkg/storage/mvcc.go Outdated Show resolved Hide resolved
pkg/storage/mvcc.go Show resolved Hide resolved
@erikgrinaker erikgrinaker force-pushed the mvcc-revert-range branch 2 times, most recently from 612dc9a to 093180b Compare February 17, 2022 15:16
Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

API as it'll be available to bulk/sql clients LGTM!

Since `RevertRange` mutates MVCC history, we want them to be alone in a
batch. The DistSender will split any batches that have multiple such
requests.

Release note: None
This patch adds a method `NextKeyIgnoringTime()` for
`MVCCIncrementalIterator`. This can be used to find the next key (as
opposed to version) of the iterator, ignoring the time bounds. It's
similar to `NextIgnoringTime()`, but calls `NextKey()` instead of
`Next()` on the underlying iterator.

Release note: None
This adds a new parameter `ExperimentalPreserveHistory` to `RevertRange`
which, rather than clearing keys above the target time, will write new
values or tombstones that reflect the state at the target time. For long
runs of deleted keys, this will instead drop an MVCC range tombstone.
This makes the command respect e.g. MVCC immutability, the closed
timestamp, and timestamp cache.

Note that MVCC range tombstones are currently experimental, and as such
this parameter is also experimental. Callers must call
`storage.CanUseExperimentalMVCCRangeTombstones()` before using it.

Release note: None
@erikgrinaker
Copy link
Contributor Author

TFTRs!

bors r=aliher1911,dt

@craig
Copy link
Contributor

craig bot commented Feb 21, 2022

Build failed:

@erikgrinaker
Copy link
Contributor Author

Some weird agent failure for git checkout.

bors retry

@craig
Copy link
Contributor

craig bot commented Feb 21, 2022

Build failed:

@erikgrinaker
Copy link
Contributor Author

Unrelated flake. Third time's the charm!

bors retry

@craig
Copy link
Contributor

craig bot commented Feb 22, 2022

Build succeeded:

@craig craig bot merged commit 4c65a10 into cockroachdb:master Feb 22, 2022
craig bot pushed a commit that referenced this pull request Feb 23, 2022
76921: storage: revert experimental MVCC range tombstones r=aliher1911 a=erikgrinaker

This reverts most of #76131, #76203, and #76478 -- except minor changes that were unrelated to the range tombstones themselves.

This leaves a gap for cluster version `Internal:78` -- I think that's probably fine, but I've left a comment.

Co-authored-by: Erik Grinaker <[email protected]>
@erikgrinaker erikgrinaker deleted the mvcc-revert-range branch July 26, 2022 11:25
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.

kvserver: MVCC-compliant RevertRange variant
4 participants