sql: resolve deadlock in waitForCacheState #29291
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
|
@andreimatei do you have any comments for this PR? Thanks |
andreimatei
previously requested changes
Sep 12, 2018
There was a problem hiding this comment.
LGTM
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/database.go, line 257 at r1 (raw file):
This method never goes to the store to resolve // the name to id mapping.
Returns 0 if the name to id mapping or the database descriptor are not in the cache.
change cockroachdb#25313 introduced getDatabaseID as the callback used in waitForCacheState. However, waitForCacheState holds on to a lock while calling its callback and can get stuck on an intent written by another transaction. The other transaction having written an intent can be trying to acquire the databaseCacheHolder lock, resulting in the system getting stuck in a deadlock. Originally, the deadlock was thought to be cause by another bug which got fixed through cockroachdb#28381, but now we know of an actual legitimate situation in which the deadlock can happen. The fix is to use another callback that will fix cockroachdb#25313 while not going to the store. fixes cockroachdb#29090 Release note: None
vivekmenezes
force-pushed
the
dropdb
branch
from
8e7593a
to
3297375
Compare
vivekmenezes
commented
Sep 12, 2018
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/database.go, line 257 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
This method never goes to the store to resolve // the name to id mapping.Returns 0 if the name to id mapping or the database descriptor are not in the cache.
Done.
|
bors r=andreimatei |
|
👎 Rejected by code reviews |
|
bors r+ |
Build failed (retrying...) |
craig bot
pushed a commit
that referenced
this pull request
Sep 12, 2018
29291: sql: resolve deadlock in waitForCacheState r=vivekmenezes a=vivekmenezes change #25313 introduced getDatabaseID as the callback used in waitForCacheState. However, waitForCacheState holds on to a lock while calling its callback and can get stuck on an intent written by another transaction. The other transaction having written an intent can be trying to acquire the databaseCacheHolder lock, resulting in the system getting stuck in a deadlock. Originally, the deadlock was thought to be cause by another bug which got fixed through #28381, but now we know of an actual legitimate situation in which the deadlock can happen. The fix is to use another callback that will fix #25313 while not going to the store. fixes #29090 Release note: None Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
change #25313 introduced getDatabaseID as the callback
used in waitForCacheState. However, waitForCacheState holds
on to a lock while calling its callback and can get stuck
on an intent written by another transaction. The other
transaction having written an intent can be trying to acquire
the databaseCacheHolder lock, resulting in the system getting
stuck in a deadlock.
Originally, the deadlock was thought to be cause by another
bug which got fixed through #28381, but now we know of
an actual legitimate situation in which the deadlock
can happen.
The fix is to use another callback that will fix #25313
while not going to the store.
fixes #29090
Release note: None