-
Notifications
You must be signed in to change notification settings - Fork 920
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
chore: lock keys for optimistic transactions #3865
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: kostas <[email protected]>
sd.local_mask |= OPTIMISTIC_EXECUTION; | ||
shard->stats().tx_optimistic_total++; | ||
|
||
RunCallback(shard); | ||
|
||
// Check state again, it could've been updated if the callback returned AVOID_CONCLUDING flag. | ||
// Only possible for single shard. | ||
if (coordinator_state_ & COORD_CONCLUDING) | ||
if (coordinator_state_ & COORD_CONCLUDING) { |
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.
@dranikpg At first I was confused by this because for execute_optimistic
to be true
we need coordinator_state_ & COORD_CONCLUDING
. However, I found out by looking at the stream family, that there are transactions that start with this state and then they might AVOID_CONCLUDING
depending on the callback result. I almost removed the if
here 😄
@@ -1060,26 +1077,25 @@ bool Transaction::ScheduleInShard(EngineShard* shard, bool execute_optimistic) { | |||
// Check if we can run immediately | |||
if (shard_unlocked && execute_optimistic && | |||
CheckLocks(GetDbSlice(shard->shard_id()), mode, lock_args)) { | |||
// We need to acquire the fp locks because the executing callback | |||
// within RunCallback might preempt. | |||
acquire_fp_locks(shard_unlocked); |
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 also thought adding a check
here and make acquire_fp_locks
return a bool. But there is no really a need since CheckLocks
gurantees that we hold none.
@dranikpg you are our transaction guy so plz let me know if I forgot anything -- highly likely 😄 |
We did not acquire any locks for transactions that are executing optimistically. However, this is problematic for callbacks that need to preempt (e.g. because a journal is active).
resolves #3841