-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
txn: protect primary locks of pessimistic transactions from being collapsed #5575
Merged
Merged
Changes from 30 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
28d09f7
protect cleanup result from being collapsed
sticnarf b8afa01
Merge branch 'master' into protect-rollback
sticnarf 93b60fa
Add cleanup and collapse rollback tests
sticnarf ee4a286
Add tests for protected field
sticnarf e844c96
Merge branch 'master' into protect-rollback
sticnarf c89b367
Set protected to false by default and add more comments
sticnarf 71aeae3
Reuse short value field for compatibility
sticnarf 857f78f
Merge branch 'master' into protect-rollback
sticnarf fa6ad05
Add unit test for is_protected
sticnarf 47eb1fa
Fix test_cleanup
sticnarf a72cde0
util: compare raw and encoded keys without explicit decoding
sticnarf 3944db5
Merge branch 'master' into compare-key
sticnarf 1953617
Merge remote-tracking branch 'upstream/master' into compare-key
sticnarf ac24c25
Add negative cases
sticnarf d8820fe
Merge branch 'master' into compare-key
sticnarf fd5784c
address comments
sticnarf 29dc972
Merge remote-tracking branch 'upstream/master' into compare-key
sticnarf 4215ba4
address comments
sticnarf 5741c61
don't panic when encoded is invalid
sticnarf 83d0e43
make clippy happy
sticnarf e161686
Merge branch 'compare-key' into protect-rollback
sticnarf 802cc5d
determine protected by checking if the key is primary
sticnarf 04819fb
fix comments for Key::is_encoded_from
sticnarf 935874c
Merge branch 'compare-key' into protect-rollback
sticnarf 70d7b1e
Merge remote-tracking branch 'upstream/master' into protect-rollback
sticnarf fbc77f3
remove protect param in cleanup
sticnarf b78d5fd
Merge remote-tracking branch 'upstream/master' into protect-rollback
sticnarf 7ea4c03
remove comments that no long hold
sticnarf b12eebb
fix incorrect comments in tests
sticnarf e5d2dc8
Merge remote-tracking branch 'upstream/master' into protect-rollback
sticnarf 26db127
update comments
sticnarf 47db423
Merge branch 'master' into protect-rollback
sticnarf 5040354
Merge branch 'master' into protect-rollback
AndreMouche File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,9 @@ const FLAG_DELETE: u8 = b'D'; | |
const FLAG_LOCK: u8 = b'L'; | ||
const FLAG_ROLLBACK: u8 = b'R'; | ||
|
||
/// The short value for rollback records which are protected from being collapsed. | ||
const PROTECTED_ROLLBACK_SHORT_VALUE: &[u8] = b"protected"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe 1 byte is enough? |
||
|
||
impl WriteType { | ||
pub fn from_lock_type(tp: LockType) -> Option<WriteType> { | ||
match tp { | ||
|
@@ -75,6 +78,7 @@ impl std::fmt::Debug for Write { | |
} | ||
|
||
impl Write { | ||
/// Creates a new `Write` record. | ||
pub fn new(write_type: WriteType, start_ts: u64, short_value: Option<Value>) -> Write { | ||
Write { | ||
write_type, | ||
|
@@ -83,6 +87,20 @@ impl Write { | |
} | ||
} | ||
|
||
pub fn new_rollback(start_ts: u64, protected: bool) -> Write { | ||
let short_value = if protected { | ||
Some(PROTECTED_ROLLBACK_SHORT_VALUE.to_vec()) | ||
} else { | ||
None | ||
}; | ||
|
||
Write { | ||
write_type: WriteType::Rollback, | ||
start_ts, | ||
short_value, | ||
} | ||
} | ||
|
||
pub fn to_bytes(&self) -> Vec<u8> { | ||
let mut b = Vec::with_capacity(1 + MAX_VAR_U64_LEN + SHORT_VALUE_MAX_LEN + 2); | ||
b.push(self.write_type.to_u8()); | ||
|
@@ -122,6 +140,15 @@ impl Write { | |
pub fn parse_type(mut b: &[u8]) -> Result<WriteType> { | ||
WriteType::from_u8(b.read_u8()?).ok_or(Error::BadFormatWrite) | ||
} | ||
|
||
pub fn is_protected(&self) -> bool { | ||
self.write_type == WriteType::Rollback | ||
&& self | ||
.short_value | ||
.as_ref() | ||
.map(|v| *v == PROTECTED_ROLLBACK_SHORT_VALUE) | ||
.unwrap_or_default() | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
@@ -165,8 +192,10 @@ mod tests { | |
fn test_write() { | ||
// Test `Write::to_bytes()` and `Write::parse()` works as a pair. | ||
let mut writes = vec![ | ||
Write::new(WriteType::Put, 0, None), | ||
Write::new(WriteType::Delete, 0, Some(b"short_value".to_vec())), | ||
Write::new(WriteType::Put, 0, Some(b"short_value".to_vec())), | ||
Write::new(WriteType::Delete, 1 << 20, None), | ||
Write::new_rollback(1 << 40, true), | ||
Write::new(WriteType::Rollback, 1 << 41, None), | ||
]; | ||
for (i, write) in writes.drain(..).enumerate() { | ||
let v = write.to_bytes(); | ||
|
@@ -183,4 +212,16 @@ mod tests { | |
assert!(Write::parse(&v[..1]).is_err()); | ||
assert_eq!(Write::parse_type(&v).unwrap(), lock.write_type); | ||
} | ||
|
||
#[test] | ||
fn test_is_protected() { | ||
assert!(Write::new_rollback(1, true).is_protected()); | ||
assert!(!Write::new_rollback(2, false).is_protected()); | ||
assert!(!Write::new( | ||
WriteType::Put, | ||
3, | ||
Some(PROTECTED_ROLLBACK_SHORT_VALUE.to_vec()) | ||
) | ||
.is_protected()); | ||
} | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in doubt about the correctness to set this non-protected. Rest LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok because if is no
Lock
exist here, then the corresponding transaction must not a pessimistic transaction's primary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've update the comments here to:
Do you think it's OK? @MyonKeminta