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

release-23.2: storage: check for replicated locks in CheckSSTConflicts #113161

Merged

Conversation

nvanbenschoten
Copy link
Member

Backport 1/1 commits from #113120.

/cc @cockroachdb/release


Informs #111984.
Informs #111893.
Informs #111530.

This commit changes storage.CheckSSTConflicts to also check for replicated locks, in addition to checking for intents. This is necessary to prevent AddSSTable from ingesting key-values pairs below replicated locks, which could violate the isolation expected from these locks if an ingested key-value is below the commit timestamp of the lock holder. This was caught in kvnemesis by the validation logic recently introduced in 7ff1c79.

The change alters the semantics of CheckSSTConflicts slightly for intents. It now detects conflicts with any intents in the AddSSTable's key span, instead of just those on overlapping keys. I think this is ok, as the checkConflicts = false path in EvalAddSSTable already had this behavior. This is reflected in the changes to cmd_add_sstable_test.go, where three cases are no longer inconsistent with blind writes. I also don't think this is a performance concern (like the comment that's being deleted was suggesting) because we now have the separated lock table. In fact, this change allows us to avoid using an intentInterleavingIter, so if anything, it may provide a small performance win.

Release note: None

Release justification: Resolves a bug in the interaction between replicated locks and AddSSTable requests.

Informs cockroachdb#111984.
Informs cockroachdb#111893.
Informs cockroachdb#111530.

This commit changes storage.CheckSSTConflicts to also check for
replicated locks, in addition to checking for intents. This is necessary
to prevent AddSSTable from ingesting key-values pairs below replicated
locks, which could violate the isolation expected from these locks if an
ingested key-value is below the commit timestamp of the lock holder.
This was caught in kvnemesis by the validation logic recently introduced
in 7ff1c79.

The change alters the semantics of CheckSSTConflicts slightly for
intents. It now detects conflicts with any intents in the AddSSTable's
key span, instead of just those on overlapping keys. I think this is ok,
as the `checkConflicts = false` path in `EvalAddSSTable` already had
this behavior. This is reflected in the changes to cmd_add_sstable_test.go,
where three cases are no longer inconsistent with blind writes. I also
don't think this is a performance concern (like the comment that's being
deleted was suggesting) because we now have the separated lock table. In
fact, this change allows us to avoid using an `intentInterleavingIter`,
so if anything, it may provide a small performance win.

Release note: None
@blathers-crl
Copy link

blathers-crl bot commented Oct 26, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Oct 26, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nvanbenschoten nvanbenschoten merged commit 20f0838 into cockroachdb:release-23.2 Oct 26, 2023
2 checks passed
@nvanbenschoten nvanbenschoten deleted the backport23.2-113120 branch November 3, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants