[SERVER-42055] Only acquire a collection IX lock to write the lastVote document Created: 02/Jul/19 Updated: 29/Oct/23 Resolved: 08/Jul/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication |
| Affects Version/s: | None |
| Fix Version/s: | 3.6.14, 4.0.11, 4.2.0-rc3, 3.4.23, 4.3.1 |
| Type: | Improvement | Priority: | Critical - P2 |
| Reporter: | Judah Schvimer | Assignee: | Matthew Russotto |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||||||
| Backport Requested: |
v4.2, v4.0, v3.6, v3.4
|
||||||||||||||||||||||||||||||||
| Sprint: | Repl 2019-07-15 | ||||||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||||||
| Case: | (copied to CRM) | ||||||||||||||||||||||||||||||||
| Linked BF Score: | 16 | ||||||||||||||||||||||||||||||||
| Description |
|
This lock acquisition should be a collection IX, not a DB X. This will prevent elections from blocking behind other local database readers and writers. This is just a single document update and as a result shouldn't need a stronger lock. Before doing this we should check the history of this lock and see why it is so coarse grained. |
| Comments |
| Comment by Matthew Russotto [ 06/Aug/19 ] |
|
Thanks for catching this; we have fixed it in |
| Comment by David Bartley [ 06/Aug/19 ] |
|
Thanks for the backport to 3.4. I might be mistaken, but it looks like https://github.com/mongodb/mongo/commit/f7d070aebfeb0be666f2f5220c970aa3bd83621e#diff-f05a3ecd80571cfd9c2798555ee39187R649 will cause storeLocalLastVoteDocument to always return early before txn->recoveryUnit()->waitUntilDurable(); is called. That's similar to the 3.6 backport, but there that return is run within a lambda (unlike 3.4, which uses macro helpers), and the return doesn't escape the lambda, and waitUntilDurable will be called. |
| Comment by Githook User [ 06/Aug/19 ] |
|
Author: {'name': 'Matthew Russotto', 'username': 'mtrussotto', 'email': 'matthew.russotto@10gen.com'}Message: |
| Comment by Githook User [ 05/Aug/19 ] |
|
Author: {'name': 'Matthew Russotto', 'email': 'matthew.russotto@10gen.com', 'username': 'mtrussotto'}Message: Backported error code CannotCreateCollection on top of cherry pick. (cherry picked from commit 7320a19db85c552a1c88204c4145f2cf18dfcb90) |
| Comment by Githook User [ 11/Jul/19 ] |
|
Author: {'name': 'Matthew Russotto', 'username': 'mtrussotto', 'email': 'matthew.russotto@10gen.com'}Message: (cherry picked from commit 7320a19db85c552a1c88204c4145f2cf18dfcb90) |
| Comment by Kelsey Schubert [ 11/Jul/19 ] |
|
Requesting backports to 3.4, because I believe this would resolve |
| Comment by Githook User [ 09/Jul/19 ] |
|
Author: {'name': 'Matthew Russotto', 'email': 'matthew.russotto@10gen.com', 'username': 'mtrussotto'}Message: (cherry picked from commit 7320a19db85c552a1c88204c4145f2cf18dfcb90) |
| Comment by Githook User [ 08/Jul/19 ] |
|
Author: {'name': 'Matthew Russotto', 'email': 'matthew.russotto@10gen.com', 'username': 'mtrussotto'}Message: |
| Comment by Matthew Russotto [ 03/Jul/19 ] |
|
Eric: I meant the deadlock on the help ticket I think an initial "never voted" with term -1 should work. |
| Comment by Andy Schwerin [ 03/Jul/19 ] |
|
I think we should go all the way to a solution that requires no X lock on the collection. Either populating the collection with an initial “never voted” document on creation, or something cleverer. |
| Comment by Eric Milkie [ 03/Jul/19 ] |
|
Do you mean "race" instead of "deadlock" here? |
| Comment by Matthew Russotto [ 03/Jul/19 ] |
|
Logically, we need a Collection X (not IX) here. The code is checking to see if the collection as a whole is empty, then checking the term on the existing document, then upserting the document assuming there's exactly 0 or 1 documents in the collection. So if Collection X is sufficient to break the deadlock we should use that. |
| Comment by Eric Milkie [ 03/Jul/19 ] |
|
Yes, I'm in favor of explicitly creating that collection on startup and simplify the other locking involved here. |
| Comment by Matthew Russotto [ 03/Jul/19 ] |
|
The only place we create local.replset.election is within putSingleton (which calls update()), which requires the X lock. So that's probably why there's a DB X lock. Most likely we can just create the collection at replication coordinator startup and change it to a collection IX. |
| Comment by Eric Milkie [ 03/Jul/19 ] |
|
This lock acquisition has been there since the beginning of the Raft implementation – Matt Dannenberg wrote the code and I reviewed it, so we must have believed at the time that it was necessary. It's 4 years later and I can't remember why I thought it was necessary then – and I agree that it isn't necessary now. |