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: handle intents under ingested range key in CheckSSTConflicts #94395

Merged
merged 1 commit into from
Dec 29, 2022

Conversation

itsbilal
Copy link
Contributor

@itsbilal itsbilal commented Dec 28, 2022

Previously, we were returning an obscure error around MVCCValueLenAndIsTombstone not being supported for interleaved intents if we encountered an intent in the engine. This change fixes that to return a more appropriate error.

This change also fixes a case where the engine iterator would unexpectedly land on a key < the sst iterator, violating an invariant in this function.

Fixes parts of #94141.

Release note: None

Epic: None

@itsbilal itsbilal requested review from a team as code owners December 28, 2022 22:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Previously, we were returning an obscure error around
MVCCValueLenAndIsTombstone not being supported for interleaved intents
if we encountered an intent in the engine. This change fixes that to return
a more appropriate error.

This change also fixes a case where the engine iterator would
unexpectedly land on a key < the sst iterator, violating an
invariant in this function.

Fixes parts of cockroachdb#94141.

Release note: None

Epic: none
@itsbilal itsbilal force-pushed the checksstconflicts-rangekey-fixes branch from 395abe5 to 745100d Compare December 29, 2022 15:42
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:

re: the backport-22.2.x label, do we actually need to backport this? MVCCValueLenAndIsTombstone does not exist on 22.2.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@erikgrinaker
Copy link
Contributor

re: the backport-22.2.x label, do we actually need to backport this? MVCCValueLenAndIsTombstone does not exist on 22.2.

Not particularly important, since we don't ingest SSTs with range keys in production code. We were planning to start testing tenant replication with serverless on 22.2, which will ingest range keys, although I doubt these will use conflict checks. Would be nice to backport any correctness fixes if it's straightforward though. As for MVCCValueLenAndIsTombstone, that can be replaced by MVCCValue.IsTombstone().

@erikgrinaker
Copy link
Contributor

Maybe I misunderstood what you meant. The problem isn't really that MVCCValueLenAndIsTombstone itself gives an unexpected error, it's that we don't handle intents correctly when ingesting range keys. The same bug exists in 22.2, and will lead to e.g. a committed-but-unresolved intent preventing the SST from being ingested.

@itsbilal
Copy link
Contributor Author

TFTRs! I'll work on the backport shortly, as it'll have to be manual.

bors r=erikgrinaker

@craig
Copy link
Contributor

craig bot commented Dec 29, 2022

Build succeeded:

@craig craig bot merged commit ed93154 into cockroachdb:master Dec 29, 2022
@blathers-crl
Copy link

blathers-crl bot commented Dec 29, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 745100d to blathers/backport-release-22.2-94395: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

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.

4 participants