[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:
Depends
Documented
is documented by DOCS-15661 Investigate changes in SERVER-69443: ... Backlog
Related
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:

  • Multi-doc txn with txnNumber:0 and readConcern: { level: 'majority' }

    returns "Given transaction number 0 does not match any in-progress transactions".

    • I believe it should return "Majority read concern is not enabled" instead.
  • Multi-doc txn with txnNumber:0 and readConcern: { level: 'majority' }

    and allowSpeculativeMajorityRead: true returns "Given transaction number 0 does not match any in-progress transactions".

    • I believe it should return "Majority read concern is not enabled" instead.
  • Multi-doc txn with txnNumber: 1 and readConcern: { level: 'majority' }

    : aborts with "Majority read concern is not enabled"

    • This is expected.
  • Multi-doc txn with txn txnNumber:1 and readConcern: { level: 'majority' }

    and allowSpeculativeMajorityRead: true crashes the server.

    • I believe it should return "Majority read concern is not enabled" instead.

 



 Comments   
Comment by Githook User [ 30/Sep/22 ]

Author:

{'name': 'ali-mir', 'email': 'ali.mir@mongodb.com', 'username': 'ali-mir'}

Message: SERVER-69443 Disallow speculative majority find operations in transactions
Branch: v4.4
https://github.com/mongodb/mongo/commit/5465c8fa07c96e06a57bbae867a065119657814f

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:

  • This invariant only triggers when emrc=false. For emrc=true cases, isSpeculativeMajority will return false, so we will never hit the invariant check. This is because isSpeculativeMajority checks that the _majorityReadMechanism is kSpeculative, and we only set the _majorityReadMechanism to kSpeculative when readConcern is majority, is the allowSpeculativeMajorityReads flag is enabled, and if emrc=false.
  • The allowSpeculativeMajorityRead flag is undocumented, so the only way a user can trigger this invariant is if they start a transaction and immediately issue a find with the undocumented field.
Generated at Thu Feb 08 06:13:28 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.