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: check for replicated locks in CheckSSTConflicts #113120

Merged

Conversation

nvanbenschoten
Copy link
Member

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

@nvanbenschoten nvanbenschoten added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Oct 26, 2023
@nvanbenschoten nvanbenschoten requested review from a team as code owners October 26, 2023 05:05
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@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.

Thanks, appreciate the simplification.

Copy link
Collaborator

@arulajmani arulajmani 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 2 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt, @nvanbenschoten, and @sumeerbhola)


pkg/storage/sst_test.go line 44 at r1 (raw file):

	testcases := []struct {
		maxLockConflicts int64
		expectIntents    []string

nit: s/expectIntents/expectLockConflicts/s while we're here?

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
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/addSSTReplLocks branch from b824e22 to ff633ba Compare October 26, 2023 15:52
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

TFTRs!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani, @dt, and @sumeerbhola)


pkg/storage/sst_test.go line 44 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: s/expectIntents/expectLockConflicts/s while we're here?

Done.

@craig
Copy link
Contributor

craig bot commented Oct 26, 2023

Build succeeded:

@craig craig bot merged commit 5a38b57 into cockroachdb:master Oct 26, 2023
3 checks passed
@blathers-crl
Copy link

blathers-crl bot commented Oct 26, 2023

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 ff633ba to blathers/backport-release-23.2-113120: 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 23.2.x failed. See errors above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants