[SERVER-31941] Disallow retryable writes in storage engines which do not support document-level locking Created: 13/Nov/17 Updated: 30/Oct/23 Resolved: 17/Nov/17 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Sharding |
| Affects Version/s: | 3.6.0-rc3 |
| Fix Version/s: | 3.6.0-rc5, 3.7.1 |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Randolph Tan | Assignee: | Jack Mulrow |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | bkp | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||
| Backport Requested: |
v3.6
|
||||||||||||||||||||||||
| Sprint: | Sharding 2017-12-04 | ||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||
| Linked BF Score: | 0 | ||||||||||||||||||||||||
| Description |
|
While retryable writes works in MMAPV1, due to the collection-level locking its performance is so bad that it cannot be used in practice. Because of this, we should disable retryable writes if StorageEngine::supportsDocLocking() == false. The effective behaviour should be that if the storage engine doesn't support document-level locking, any requests which include a txnNumber should fail with code UnsupportedOperation. |
| Comments |
| Comment by Githook User [ 21/Nov/17 ] | ||||||||||||||||
|
Author: {'name': 'Jack Mulrow', 'username': 'jsmulrow', 'email': 'jack.mulrow@mongodb.com'}Message: (cherry picked from commit aacd508f5092b433df5bf0ea0e1c03844fffc2bb) | ||||||||||||||||
| Comment by Githook User [ 21/Nov/17 ] | ||||||||||||||||
|
Author: {'name': 'Jack Mulrow', 'username': 'jsmulrow', 'email': 'jack.mulrow@mongodb.com'}Message: | ||||||||||||||||
| Comment by Githook User [ 17/Nov/17 ] | ||||||||||||||||
|
Author: {'name': 'Jack Mulrow', 'username': 'jsmulrow', 'email': 'jack.mulrow@mongodb.com'}Message: (cherry picked from commit 97af8701b538754261e566b26fa22cb4b54710f3) | ||||||||||||||||
| Comment by Githook User [ 17/Nov/17 ] | ||||||||||||||||
|
Author: {'name': 'Jack Mulrow', 'username': 'jsmulrow', 'email': 'jack.mulrow@mongodb.com'}Message: | ||||||||||||||||
| Comment by Randolph Tan [ 15/Nov/17 ] | ||||||||||||||||
|
shane.harvey They should be harmless. The secondary application of these oplog entries should have minimal difference in terms of overhead when compared to WiredTiger. There would be a huge performance impact on migrations if the user is using mixed storage engine shards and the mmap becomes the primary during migraitons though. | ||||||||||||||||
| Comment by Shane Harvey [ 14/Nov/17 ] | ||||||||||||||||
|
txnNumbers show up in the oplog entries for write commands:
What happens when an MMAPv1 secondary applies such an oplog entry from a WiredTiger primary? | ||||||||||||||||
| Comment by Kaloian Manassiev [ 14/Nov/17 ] | ||||||||||||||||
|
After further discussion, it appears that the requested retryableWrites field on the isMaster response counts as a "feature test" and it is the general consensus on the server that there is no good way to support that in a sharded cluster due to the possibility of the constituent nodes to change. Because of this we are not going to add the "retryableWrites" field on isMaster. The driver should just send the retries blindly as it does now and if the node doesn't support them so be it. | ||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 13/Nov/17 ] | ||||||||||||||||
|
This is consistent with our philosophy about Retryable Writes so far: if the URI contains "retryWrites=true" then drivers retry all operations they can but don't error if there are operations they can't. So if the operation isn't retryable (dropDatabase) or the server doesn't support retries (standalone, MongoDB 3.4) then we ignore the "retryWrites" setting. Doing the same thing for MMAPv1 is consistent. | ||||||||||||||||
| Comment by Kaloian Manassiev [ 13/Nov/17 ] | ||||||||||||||||
|
I don't think it is a problem to add it to master as part of this commit. It will be the same check that we need to do for the actual operations, so it's fairly simple. | ||||||||||||||||
| Comment by Bernie Hackett [ 13/Nov/17 ] | ||||||||||||||||
|
I don't see how we have a choice. How quickly can it be added to ismaster? | ||||||||||||||||
| Comment by Kaloian Manassiev [ 13/Nov/17 ] | ||||||||||||||||
|
Add a retryableWrites field to the isMaster response sounds like a reasonable solution. Is this something that can be incorporated in all the drivers by GA time? cc alyson.cabral. | ||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 13/Nov/17 ] | ||||||||||||||||
|
I propose a new isMaster response field, "retryableWrites: <bool>". It's false if sessions are not supported (so it's false if logicalSessionTimeoutMinutes is absent) and it's also false if the storage engine doesn't support document-level locking. | ||||||||||||||||
| Comment by Bernie Hackett [ 13/Nov/17 ] | ||||||||||||||||
|
How will this present to drivers? We determine support for retryable writes by server support for sessions. Specifically if the ismaster command returns the logicalSessionTimeoutMinutes field. |