[SERVER-78228] _decisionDurable flag is not set correctly when continuing two-phase commit after step up Created: 20/Jun/23 Updated: 12/Dec/23 |
|
| Status: | Open |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Ivan Fefer | Assignee: | Brett Nawrocki |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | cs-subteam1, sharding-nyc-subteam1 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Assigned Teams: |
Sharding NYC
|
| Operating System: | ALL |
| Participants: | |
| Story Points: | 5 |
| Description |
|
When persisting decision we check for _decisionDurable flag, because if we are continuing the commit after step up, there is no need to persist decision again. However, we are not setting this flag in continueCommit: we should add _decisionDurable = true if TransactionCoordinatorDocument has a decision. This is not a correction issue, we just waste some time by writing the decision again. |
| Comments |
| Comment by Max Hirschhorn [ 22/Jun/23 ] | ||
|
To add more detail about the subtlety - The logic which returns a null optime means the WaitForMajorityService will be trivially satisfied by the primary.
This is problematic because the primary does not readily know whether the last write to the transaction coordinator state document has been majority-committed or not if the node was the primary in the term when the write was initially committed. Generally the solution to this problem involves waiting for the latest optime to become majority-committed by waiting on the optime from ReplClientInfo::setLastOpToSystemLastOpTime(). In a 5-node replica set taking the if-branch would lead to the following unsafe behavior:
Because the primary still attempts to update the transaction coordinator state document (it won't match a document if the decision has already been made) the primary ends up advancing the OperationContext to track the latest optime through the LastOpFixer::~LastOpFixer() destructor. | ||
| Comment by Ivan Fefer [ 20/Jun/23 ] | ||
|
This can be more tricky than it seems, so maybe we should allow it to happen and remove misleading comment and always-false if check. |