sql: add missing locking for txnState.mu.txn writes #33249
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
Fixes cockroachdb#32523 Release note: None
|
This change is |
|
Note that I'm still trying to reproduce the original failure, though I'm pretty sure this is the fix. |
andreimatei
approved these changes
Dec 18, 2018
| // Note that reads of mu.txn from the session's main goroutine do not require | ||
| // acquiring a read lock - since only that goroutine will ever write to | ||
| // mu.txn. | ||
| // mu.txn. Writes to mu.txn do require a write lock to guarantee safety with |
There was a problem hiding this comment.
I'd argue that this comment is not needed, but I guess I've forfeited that right.
|
|
|
No reproduction via |
|
bors r=andreimatei |
craig bot
pushed a commit
that referenced
this pull request
Dec 18, 2018
33193: sql: remove maps from cfetcher hot path r=jordanlewis a=jordanlewis Closes #32968. Previously, the cfetcher used a Go map to keep track of the relationship between column id and column index, so that during decoding of value columns, it could put the decoded value in the correct spot depending on its column id. This was fairly expensive and showed up very heavily in profiles. This commit changes the approach used, by recognizing that within any given key/value pair, the encountered column ids will be strictly increasing. Thanks to this knowledge, it becomes cheap to use a regular list for storage of the mapping - instead of an O(n) lookup for every column value in a kv pair, it's O(n) for the whole kv pair, since we can save our position within the sorted list of required column ids. This reduces the time spent in the cfetcher's ProcessValueBytes method from 30% to 10% for a large scan. Release note: None 33248: diagnosticspb: restore deleted fields r=dt a=dt We don't actually want to delete fields since older nodes will still be writing them, and we use the same proto to decode reported diagnostics data so it wants to read those. Instead we can just prefix them so it is clear they're not used. Release note: none 33249: sql: add missing locking for txnState.mu.txn writes r=andreimatei a=petermattis Fixes #32523 Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com> Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Peter Mattis <petermattis@gmail.com>
Build succeeded |
This was referenced Dec 19, 2018
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.
Fixes #32523
Release note: None