[SERVER-16337] WriteBatchExecutor's WriteConflictException not caught Created: 26/Nov/14  Updated: 02/Feb/15  Resolved: 22/Jan/15

Status: Closed
Project: Core Server
Component/s: Storage
Affects Version/s: None
Fix Version/s: 3.0.0-rc6

Type: Bug Priority: Major - P3
Reporter: Igor Canadi Assignee: Eliot Horowitz (Inactive)
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Tested
Backwards Compatibility: Fully Compatible
Participants:

 Description   

Steps to repro:
1. throw WriteConflictException() in IndexBulkBuilder::addKey()
2. run jstests/parallel/fsm_all.js (it breaks on indexed_insert_ordered_bulk.js)



 Comments   
Comment by Githook User [ 22/Jan/15 ]

Author:

{u'username': u'erh', u'name': u'Eliot Horowitz', u'email': u'eliot@10gen.com'}

Message: SERVER-16337: retry WriteConflictException on insert path
Branch: master
https://github.com/mongodb/mongo/commit/ab9ac2126075fec83c579113bc0c81ec08b797de

Comment by Eliot Horowitz (Inactive) [ 22/Jan/15 ]

going to do this to make some things easier at various layers

Comment by Leif Walsh [ 05/Dec/14 ]

igor yep, us too: https://github.com/leifwalsh/mongo/commit/44fee88

Comment by Igor Canadi [ 05/Dec/14 ]

Rocks implementation is currently fixing that by not throwing false-positive WCE and returning dupKeyError when we detect write-write conflict in Index insert (both with committed and uncommitted transactions). Patch is here: https://github.com/mongodb-partners/mongo/commit/543e5d812fe1ba396ce5db127686808e2f25ff60

We're trying to do 'whatever wiredtiger does' for our storage engine behavior to avoid being blocked on waiting for mongo's higher level code changes. We might rethink our design if the conditions change.

Comment by Leif Walsh [ 05/Dec/14 ]

What's the resolution on this? If it really is the case that inserts never conflict, such a retry loop would be free, and it makes it easier to reason about the storage engine if we don't have to worry about when it is or isn't safe to throw a WriteConflictException. Plus, it prevents false positive duplicate key errors. Seems like a tie-win-win. I'm not sure what towel-throwing is involved here.

Comment by Siying Dong [ 01/Dec/14 ]

Eliot, can we throw dupKeyError when there is not duplicated key as a false positive case? Currently we implement in a way to hash keys to fixed number of shared buckets to detect conflicts. If we can throw dupKeyError in those false conflicts, it will be more convenient for us.

Comment by Igor Canadi [ 01/Dec/14 ]

Ok, we can return DuplicateKey when there is a conflict with uncommitted transaction. I found this in the WiredTiger code:

if ( ret == WT_ROLLBACK && !dupsAllowed )

{ // if there is a conflict on a unique key, it means there is a dup key // even if someone else is deleting at the same time, its ok to fail this // insert as a dup key as it a race return dupKeyError(key); }

Since this seems to be the only correct behavior at this point, would it make sense to write a unit test for this? It's much easier to debug a small unit test failure rather than integration test with 30 concurrent threads. We would also greatly appreciate unit tests for other tricky behaviors that are not clearly documented.

Comment by Eliot Horowitz (Inactive) [ 29/Nov/14 ]

I'm not sure it makes sense.
We've gone to some great lengths to make sure inserts don't conflict and that has some really nice properties.
Not ready to just throw in the towel on that, certainly not at this moment.

Comment by Igor Canadi [ 28/Nov/14 ]

I think we would still like to throw WriteConflictException() in Index::insert (for unique indexes). There are few reasons:
1. Our current implementation isn't exact when throwing WriteConflictException. We shard the written keys and we throw if there's a conflict in shards. While DuplicateKey error needs to be correct, WriteConflictException just means re-try again, which impacts performance instead of correctness, right?
2. Returning DuplicateKey for a conflict in uncommitted transaction also doesn't feel right. I would much rather throw WriteConflictException in that case.

Does this make sense?

Comment by Igor Canadi [ 28/Nov/14 ]

Should index insert return duplicate key exception even when the duplicate
key is in uncommitted transaction?
On Thu, Nov 27, 2014 at 9:30 PM Eliot Horowitz (JIRA) <jira@mongodb.org>

Comment by Eliot Horowitz (Inactive) [ 28/Nov/14 ]

I don't believe that catching WriteConflictException in insert should be needed.
The only case where anything should touch the same keys is unique keys, and those need to return a duplicate key exception, not a write conflict exception.

Comment by Igor Canadi [ 28/Nov/14 ]

Eliot – I originally misattributed the error to IndexBulkBuilder, but the actual error was in BulkExecutor. I fixed the problem by adding `catch(WriteConflictException)` here: https://github.com/mongodb/mongo/blob/16a8ef7ad60d498b69bdc0ad5cbca44757d16fd8/src/mongo/db/commands/write_commands/batch_executor.cpp#L1060, but not sure if that's the right solution. I'm wondering how wiredtiger doesn't fail this.

Comment by Eliot Horowitz (Inactive) [ 27/Nov/14 ]

why would there even be a conflict in bulk building?
The only case that i've seen is duplicate keys, and those should throw duplicate key exceptions, not WriteConflictExceptions

Comment by Igor Canadi [ 26/Nov/14 ]

I think `catch WriteConflictException()` needs to be added here: https://github.com/mongodb/mongo/blob/16a8ef7ad60d498b69bdc0ad5cbca44757d16fd8/src/mongo/db/commands/write_commands/batch_executor.cpp#L1060

Comment by Igor Canadi [ 26/Nov/14 ]

Sorry, this seems to be WriteBatchExecutor's code path rather than IndexBulkBuilder::addKey(). More specifically, I think this is the trace:

mongod(_ZN5mongo19RocksSortedDataImpl6insertEPNS_16OperationContextERKNS_7BSONObjERKNS_7DiskLocEb+0x538) [0xd77278]
mongod(_ZN5mongo22BtreeBasedAccessMethod6insertEPNS_16OperationContextERKNS_7BSONObjERKNS_7DiskLocERKNS_19InsertDeleteOptionsEPl+0x193) [0xab54f3]
mongod(_ZN5mongo12IndexCatalog12_indexRecordEPNS_16OperationContextEPNS_17IndexCatalogEntryERKNS_7BSONObjERKNS_7DiskLocE+0x6E) [0x96269e]
mongod(_ZN5mongo12IndexCatalog11indexRecordEPNS_16OperationContextERKNS_7BSONObjERKNS_7DiskLocE+0x56) [0x9629b6]
mongod(_ZN5mongo10Collection15_insertDocumentEPNS_16OperationContextERKNS_7BSONObjEb+0xB3) [0x950843]
mongod(_ZN5mongo10Collection14insertDocumentEPNS_16OperationContextERKNS_7BSONObjEb+0x70) [0x951e30]
mongod(_ZN5mongo18WriteBatchExecutor13execOneInsertEPNS0_16ExecInsertsStateEPPNS_16WriteErrorDetailE+0x28F) [0x9ebc6f]
mongod(_ZN5mongo18WriteBatchExecutor11execInsertsERKNS_21BatchedCommandRequestEPSt6vectorIPNS_16WriteErrorDetailESaIS6_EE+0x250) [0x9ec180]
mongod(_ZN5mongo18WriteBatchExecutor11bulkExecuteERKNS_21BatchedCommandRequestEPSt6vectorIPNS_19BatchedUpsertDetailESaIS6_EEPS4_IPNS_16WriteErrorDetailESaISB_EE+0x34) [0x9ed544]
mongod(_ZN5mongo18WriteBatchExecutor12executeBatchERKNS_21BatchedCommandRequestEPNS_22BatchedCommandResponseE+0x3A5) [0x9edc55]
mongod(_ZN5mongo8WriteCmd3runEPNS_16OperationContextERKSsRNS_7BSONObjEiRSsRNS_14BSONObjBuilderEb+0x169) [0x9efdb9]
mongod(_ZN5mongo12_execCommandEPNS_16OperationContextEPNS_7CommandERKSsRNS_7BSONObjEiRSsRNS_14BSONObjBuilderEb+0x34) [0xa09d04]
mongod(_ZN5mongo7Command11execCommandEPNS_16OperationContextEPS0_iPKcRNS_7BSONObjERNS_14BSONObjBuilderEb+0xC25) [0xa0ab65]
mongod(_ZN5mongo12_runCommandsEPNS_16OperationContextEPKcRNS_7BSONObjERNS_11_BufBuilderINS_16TrivialAllocatorEEERNS_14BSONObjBuilderEbi+0x2A2) [0xa0b6d2]
mongod(_ZN5mongo11newRunQueryEPNS_16OperationContextERNS_7MessageERNS_12QueryMessageERNS_5CurOpES3_b+0x75A) [0xbec07a]
mongod(_ZN5mongo16assembleResponseEPNS_16OperationContextERNS_7MessageERNS_10DbResponseERKNS_11HostAndPortEb+0xB15) [0xae24a5]
mongod(_ZN5mongo16MyMessageHandler7processERNS_7MessageEPNS_21AbstractMessagingPortEPNS_9LastErrorE+0xE0) [0x84c260]
mongod(_ZN5mongo17PortMessageServer17handleIncomingMsgEPv+0x411) [0xf14dd1]

Generated at Thu Feb 08 03:40:42 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.