chore: lock keys for optimistic transactions #3865
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.
@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 😄
There was a problem hiding this comment.
However, I found out by looking at the stream family,
You could've just looked at the comment above 😂 I don't write them just for fun
src/server/transaction.cc
Outdated
| @@ -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.
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.
There was a problem hiding this comment.
It seems like if we acquire locks, we can remove CheckLocks. Instead the condition will use lock_granted
There was a problem hiding this comment.
Yes, the code here should be rewritten. We acquire locks in any case, so CheckLocks is not needed
|
@dranikpg you are our transaction guy so plz let me know if I forgot anything -- highly likely 😄 |
| 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.
However, I found out by looking at the stream family,
You could've just looked at the comment above 😂 I don't write them just for fun
src/server/transaction.cc
Outdated
| @@ -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.
It seems like if we acquire locks, we can remove CheckLocks. Instead the condition will use lock_granted
Signed-off-by: kostas <[email protected]>
src/server/transaction.cc
Outdated
| // We need to acquire the fp locks because the executing callback | ||
| // within RunCallback below might preempt. | ||
| acquire_fp_locks(shard_unlocked); |
There was a problem hiding this comment.
nits:
- Maybe we don't need acquire_fp_locks as a separate function
- If you decide to keep it, maybe return lock_granted, otherwise the flow might be somewhat confusing, looking at the if below (i.e. not clear where lock_granted changed)
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