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: rewrite CheckSSTConflicts #99657

Open
jbowens opened this issue Mar 27, 2023 · 1 comment
Open

storage: rewrite CheckSSTConflicts #99657

jbowens opened this issue Mar 27, 2023 · 1 comment
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-storage Storage Team

Comments

@jbowens
Copy link
Collaborator

jbowens commented Mar 27, 2023

The current implementation of the storage.CheckSSTConflicts function is very complicated, and it's very difficult to reason about its correctness. We've uncovered many bugs (eg, #99566) in its implementation as a result, and today largely rely on randomized testing (from KV nemesis and the unit test in #98408) to ensure correctness. We should rewrite and simplify it.

Some ideas:

  • @itsbilal suggested a four-iterator approach. We'd use 1 range key iterator and 1 point iterator for both the engine and the sstable.
  • We could implement a "tandem" iterator that keeps the engine iterator positioned appropriately as the sstable iterator moves across the keyspace. There are some subtleties in what "positioned appropriately" means (eg, the engine iterator may need to read before the sstable iterator's position in order to observe a range key). Separating out the mechanics of keeping the two iterators in sync should make the actual conflict checking more tractable.

Jira issue: CRDB-26007

@jbowens jbowens added A-storage Relating to our storage engine (Pebble) on-disk storage. T-storage Storage Team labels Mar 27, 2023
@blathers-crl
Copy link

blathers-crl bot commented Mar 27, 2023

Hi @jbowens, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Mar 27, 2023
@jbowens jbowens added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Mar 27, 2023
@jbowens jbowens moved this to 24.2 candidates in [Deprecated] Storage Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-storage Storage Team
Projects
No open projects
Status: 24.2 candidates
Development

No branches or pull requests

1 participant