[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: |
|
||||
| Backwards Compatibility: | Fully Compatible | ||||
| Participants: | |||||
| Description |
|
Steps to repro: |
| 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: |
| 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. |
| 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: Does this make sense? |
| Comment by Igor Canadi [ 28/Nov/14 ] |
|
Should index insert return duplicate key exception even when the duplicate |
| Comment by Eliot Horowitz (Inactive) [ 28/Nov/14 ] |
|
I don't believe that catching WriteConflictException in insert should be needed. |
| 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? |
| 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] |