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

concurrency: allow shared locking requests into the lock table #107550

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

arulajmani
Copy link
Collaborator

@arulajmani arulajmani commented Jul 25, 2023

This patch extends the list of permissible locking strengths to include
SHARED and tests conflict resolution for shared locks with other lock
strengths.

Note that (currently) the lock table doesn't support multiple locks from
different transactions on a single key; as such, 2 transactions cannot
simultaneously hold shared locks on a single key. This restriction will
be lifted in the future.

Release note: None

@arulajmani arulajmani requested a review from a team as a code owner July 25, 2023 17:37
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@arulajmani arulajmani force-pushed the sl-accept-shared-locks branch from 056edfb to 6a92f50 Compare July 26, 2023 14:24
Copy link
Member

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

That was easy!

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)


pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 36 at r4 (raw file):


# req3 should not actively wait, as it's locking strength is shared, but it
# should be able to acquire a joint claim.

Do joint claims work yet? If so, do you want to add to this test to demonstrate that?


pkg/kv/kvserver/concurrency/lock_table_test.go line 718 at r4 (raw file):

		case lock.Shared:
			sa = spanset.SpanReadOnly
			ts = hlc.MaxTimestamp

Consider leaving a comment that points to #102264.

This patch extends the list of permissible locking strengths to include
SHARED and tests conflict resolution for shared locks with other lock
strengths.

Note that (currently) the lock table doesn't support multiple locks from
different transactions on a single key; as such, 2 transactions cannot
simultaneously hold shared locks on a single key. This restriction will
be lifted in the future.

Release note: None
@arulajmani arulajmani force-pushed the sl-accept-shared-locks branch from 6a92f50 to 292c54e Compare July 31, 2023 15:16
Copy link
Collaborator Author

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 36 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do joint claims work yet? If so, do you want to add to this test to demonstrate that?

They don't work yet because of this TODO that we spoke about in a different context last week:

// TODO(arul): Inactive waiters will need to capture the strength at which
// they're trying to acquire a lock in their queuedGuard. We can't simply
// use the guard's curStrength (or curLockMode) -- inactive waiters may have
// mutated these values as they scan. For now, we can just use the intent
// lock mode as that's the only lock strength supported by the lock table.

I'll add a test once we've addressed this.

Copy link
Member

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

:lgtm:

Reviewed 34 of 34 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani)

@arulajmani
Copy link
Collaborator Author

TFTR!

bors r=nvanbenschoten

@craig
Copy link
Contributor

craig bot commented Jul 31, 2023

Build succeeded:

@craig craig bot merged commit 45cd1c9 into cockroachdb:master Jul 31, 2023
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.

3 participants