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

sql: crdb_internal.leases could cause a deadlock with the lease manager #119305

Merged
merged 1 commit into from Feb 17, 2024

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Feb 16, 2024

Previously, a deadlock could occur between crdb_internal.leases and the lease manager when attempting to renew or release leases on the role_member inside crdb_internal.leases table. This deadlock occurs because lease manager lock is held while visting leases. The issue was easily reproducible when the
allow_role_memberships_to_change_during_transaction setting was enabled. To resolve this, the patch introduces in-memory caching of all leases within crdb_internal.leases before privilege checks are performed, and any locks are subsequently released.

Fixes: #119253
Fixes: #97280

Release note (bug fix): crdb_internal.leases could cause a node to become unavailable due to a deadlock in the leasing subsystem.

@fqazi fqazi requested a review from a team as a code owner February 16, 2024 16:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@fqazi fqazi added backport-22.2.x Flags PRs that need to be backported to 22.2. backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Feb 16, 2024
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for the quick fix, and great investigation for it

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


pkg/sql/crdb_internal.go line 919 at r1 (raw file):

	generator:     nil,
	unimplemented: false,
	resultColumns: nil,

super nit: do we need these extra nil fields defined?

Previously, a deadlock could occur between crdb_internal.leases and the
lease manager when attempting to renew or release leases on the
role_member inside crdb_internal.leases table. This deadlock occurs
because lease manager lock is held while visting leases. The issue was
easily reproducible when the
allow_role_memberships_to_change_during_transaction setting was enabled.
To resolve this, the patch introduces in-memory caching of all leases
within crdb_internal.leases before privilege checks are performed, and
any locks are subsequently released.

Fixes: cockroachdb#119253

Release note (bug fix): crdb_internal.leases could cause a node to
become unavailable due to a deadlock in the leasing subsystem.
Copy link
Collaborator Author

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

@rafiss TFTR!

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


pkg/sql/crdb_internal.go line 919 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

super nit: do we need these extra nil fields defined?

Done.

@fqazi
Copy link
Collaborator Author

fqazi commented Feb 17, 2024

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 17, 2024

Build succeeded:

@craig craig bot merged commit e39dafe into cockroachdb:master Feb 17, 2024
9 of 13 checks passed
Copy link

blathers-crl bot commented Feb 17, 2024

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 10618f3 to blathers/backport-release-22.2-119305: 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.


error creating merge commit from 10618f3 to blathers/backport-release-23.1-119305: 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.1.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-22.2.x Flags PRs that need to be backported to 22.2. backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
3 participants