[SERVER-69443] [4.4] Allow speculative majority reads in multi-doc txns when --enableMajorityReadConcern=false Created: 05/Sep/22 Updated: 29/Oct/23 Resolved: 30/Sep/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 4.4.18 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Josef Ahmad | Assignee: | Ali Mir |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||
| Operating System: | ALL | ||||||||||||||||
| Sprint: | Repl 2022-09-19, Repl 2022-10-03 | ||||||||||||||||
| Participants: | |||||||||||||||||
| Linked BF Score: | 102 | ||||||||||||||||
| Description |
|
Affects 4.4 (observed on 4.4.17-rc0), doesn't seem to affect 4.2 (doesn't reproduce on 4.2.22). Doesn't affect 5.0+ as support for majority read concern is always on. Behaviour:
|
| Comments |
| Comment by Githook User [ 30/Sep/22 ] |
|
Author: {'name': 'ali-mir', 'email': 'ali.mir@mongodb.com', 'username': 'ali-mir'}Message: |
| Comment by Ali Mir [ 16/Sep/22 ] |
|
Took a quick look at why this issue isn't reproducible on 4.2. It appears that regardless of the "majority" read concern passed into the find command in the transaction, we override it and replace it with readConcern: snapshot here. As we can only be in speculative majority if the readConcern is majority, we skip the invariant entirely. This functionality was removed in 4.4. As a result, the fix only has to go into 4.4 (as versions >= 5.0 has emrc=true). |
| Comment by Ali Mir [ 13/Sep/22 ] |
|
As a note (to myself) we'll need to prevent speculative majority finds from running in transactions in the fuzzer. |
| Comment by Kyle Suarez [ 13/Sep/22 ] |
|
After consulting with christopher.harris@mongodb.com, it sounds fine to Query to ban these speculative majority finds in transactions. However, please run the usual patch builds and let Query know if anything is red (we don't expect anything to break.... but better safe than sorry). |
| Comment by Ali Mir [ 08/Sep/22 ] |
|
Question for query execution: are there any concerns with banning speculative majority find commands in transactions? Speculative majority finds and transactions seem to have conflicting behavior with regards to ReadSource, and we could uassert in the find code path. It seems to me that this would be a low disruption change, given that really the only case we are banning is a find command with speculative majority (and the field is internal/undocumented) as the first operation in a transaction. cc kyle.suarez@mongodb.com (feel free to tag in anyone that might have more context!). Let me know if there is something I'm missing from my investigation, or if you want to chat more about this! |
| Comment by Ali Mir [ 08/Sep/22 ] |
|
I've confirmed that transactions do not support the $changeStream aggregation stage (function here) by running a jstest. It looks like the find command case is the only command that can trigger the invariant within a transaction on v4.4. |
| Comment by Ali Mir [ 08/Sep/22 ] |
|
It appears that with even with emrc=false, find commands with readConcern: majority and allowSpeculativeMajorityReads: true should still succeed. We use the allowSpeculativeMajorityReads flag to provide 'majority' read guarantees even when the storage engine doesn't support reading from historical snapshot, as described by this test on the v4.4 branch. As a result, just returning a "Majority read concern is not enabled" error for allowSpeculativeMajorityReads flagged commands in a transaction is a little misleading. The failure arises because of the invariant check here. We expect the ReadSource of any speculative majority read command to be kNoOverlap. However, we will always set the ReadSource to kNoTimestamp for transactions with readConcern local or majority. Therefore, any transaction with readConcern:majority, the speculative majority flag, and emrc=false will trigger this invariant. To me, it sounds like conflicting behavior, so my proposal is that we ban speculative majority commands from transactions entirely. Given that this is an internal only field, and is used only in two places (according to this comment, in changestreams and the find command), it shouldn't be too disruptive of a change. Some other notes:
|