[SERVER-56877] insert operations may fail to set index to multikey after aborted multikey catalog update Created: 12/May/21 Updated: 29/Oct/23 Resolved: 20/Jul/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Storage |
| Affects Version/s: | None |
| Fix Version/s: | 5.0.2, 4.4.9, 5.1.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Benety Goh | Assignee: | Benety Goh |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Operating System: | ALL | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Backport Requested: |
v5.0, v4.4, v4.2, v4.0
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sprint: | Execution Team 2021-05-31, Execution Team 2021-06-14 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Linked BF Score: | 127 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
When a storage transaction to update an index to multikey is aborted, it is possible for the in-memory state of an index to appear as multikey but the durable state to remain as non-multikey. In this scenario, the IndexCatalogEntryImpl (erroneous in-memory state) may contain path-level information which lead to this optimization being erroneously applied to subsequent multikey writes and leaving the index as non-multikey in the catalog. |
| Comments |
| Comment by Vivian Ge (Inactive) [ 06/Oct/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Updating the fixversion since branching activities occurred yesterday. This ticket will be in rc0 when it’s been triggered. For more active release information, please keep an eye on #server-release. Thank you! | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 12/Aug/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This change applies the technique in commit 3566db153ea61fb10d3ef11ea917fc7bc93eac4d This commit is designed to support pre-5.0 backports and will be superseded by a (cherry picked from commit 5d8ac7127e207e0ef1aeaa90dcc62cda6896b71e) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 10/Aug/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: (cherry picked from commit 262036ae4b0219c229ff38c644a1e10487e7a586) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 10/Aug/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that the multikey paths for a compound index remain consistent This initial version of the test characterizes a defect in multikey tracking that shows (cherry picked from commit 54f015ab1bc386b61c8011d5d8e3267bb4e181bf) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 10/Aug/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that a full validation runs successfully on a collection with a multikey This initial version of the test characterizes a defect in multikey state tracking that (cherry picked from commit 393465e9fe428e693c259223c85299f8e7aac1fc) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 10/Aug/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that the multikey state of an index is updated correctly in the durable This initial version of the test characterizes an defect in tracking the multikey information (cherry picked from commit aefb1ac44425c13e8803ff72a3c7fcb194ee599c) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 21/Jul/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This change depends on improvements made in commit 11de948b0c50df7d12de09ae0f01e791fc5d70d7 and (cherry picked from commit bdb1299412486fab396214f5745d1768a56a449f) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 20/Jul/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This change applies the technique in commit 3566db153ea61fb10d3ef11ea917fc7bc93eac4d This commit is designed to support pre-5.0 backports and will be superseded by a (cherry picked from commit 5d8ac7127e207e0ef1aeaa90dcc62cda6896b71e) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 20/Jul/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This reverts commit fcddb1393fa511f68706b1ff8e74c65f40c086bc. (cherry picked from commit aefb1ac44425c13e8803ff72a3c7fcb194ee599c) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 02/Jun/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This change depends on improvements made in commit 11de948b0c50df7d12de09ae0f01e791fc5d70d7 and | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 02/Jun/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This change applies the technique in commit 3566db153ea61fb10d3ef11ea917fc7bc93eac4d This commit is designed to support pre-5.0 backports and will be superseded by a | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 25/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: Revert " This reverts commit f73a9e99f3f1b9def1087904ddc6891952e62efc. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Benety Goh [ 24/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
| |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Benety Goh [ 24/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Backports to 4.2 and 4.0 may have to take into account minor edits to IndexCatalogEntryImpl made in | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Benety Goh [ 21/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
The main issue with the reverted commit bc095d5c is in this line where we unconditionally replace the read paths with the last successfully persisted write paths. We are not taking into account the compound index case where we may be updating separate paths, from different documents, in a single storage transaction. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 21/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: Revert " This reverts commit c1c9bafde288414adf94b3f8372189e38b2f8e0f. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 21/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: Revert " This reverts commit bc095d5cef314bab9fd583c3908eb1cfecb49dd9. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 20/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: (cherry picked from commit bc095d5cef314bab9fd583c3908eb1cfecb49dd9) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 20/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: (cherry picked from commit 262036ae4b0219c229ff38c644a1e10487e7a586) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 20/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that the multikey paths for a compound index remain consistent This initial version of the test characterizes a defect in multikey tracking that shows (cherry picked from commit 54f015ab1bc386b61c8011d5d8e3267bb4e181bf) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 20/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that a full validation runs successfully on a collection with a multikey This initial version of the test characterizes a defect in multikey state tracking that (cherry picked from commit 393465e9fe428e693c259223c85299f8e7aac1fc) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 20/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that the multikey state of an index is updated correctly in the durable This initial version of the test characterizes an defect in tracking the multikey information (cherry picked from commit aefb1ac44425c13e8803ff72a3c7fcb194ee599c) | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Benety Goh [ 20/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
| |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 19/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 19/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that the multikey paths for a compound index remain consistent This initial version of the test characterizes a defect in multikey tracking that shows | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 19/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that a full validation runs successfully on a collection with a multikey This initial version of the test characterizes a defect in multikey state tracking that | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 19/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: This test checks that the multikey state of an index is updated correctly in the durable This initial version of the test characterizes an defect in tracking the multikey information | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 18/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Daniel Gottlieb (Inactive) [ 18/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
This to me is the natural progression of the solution. Each multikey path gets a read variable and a write variable (which probably means some sort of set/map of paths). The corresponding read variable is pessimistically set before the accompanying write is committed. Even if the write is rolled back, the variable stays set. This variable determines whether the index/path is multikey for query's purpose. The corresponding write variable is set in an onCommit hook. Every write that has multikey paths must perform a write to the catalog to set its corresponding multikey path until this variable is observed to be set. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Louis Williams [ 13/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Since this "versioned catalog" project might be coming soon, I think we should consider deferring this bug fix until we understand how much longer we have to wait for a solution that makes all of this in-memory multikey maintenance code go away. That is, unless this bug fix is simple. Based on the discussion here, that does not seem to be the case. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Daniel Gottlieb (Inactive) [ 13/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
Yep. I'd still consider that the same bug, but it's certainly true that checking "is multikey for write" will eliminate a case that can hit the bug.
Correct. The interleaving that's problematic:
That's exactly what it is. If we could atomically change in-memory state with the storage engine, we wouldn't need all these tricks. The lock free reads project has given us a lot of the code changes necessary to move to a "versioned catalog". To me, that means that whenever we get a collection object, we actually use the snapshot on the recovery unit to read the collection metadata from the _mdb_catalog table and hydrate a (relatively) immutable collection object from that data. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Benety Goh [ 12/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
If I understand your example with paths 'a' and 'b', that scenario is still an issue today. I wonder what the effect of guarding the optimization with _isMultikeyKeyForWrite and installing a rollback handler to restore the state of _indexMultikeyPaths here would be on the scenario you described. I understand that, even with the ability to restore the previous version of _indexMultikeyPaths, we will still not be able to resolve all the possible interleaving cases. Most of the logic for the in-memory path maintenance appears to be some approximation of the logic we use to update the durable state in DurableCatalogImpl::setIndexIsMultiKey() | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Daniel Gottlieb (Inactive) [ 12/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Talking about solutions to these are tricky, so I've diagramed a few interleavings: My understanding of how (the bug) happens today:
My understanding of how checking for _isMultikeyForWrite addresses the prior example (correctly):
I claim that checking _isMultikeyForWrite isn't sufficient because it's not granular enough. Flipping _isMultikeykeyForWrite when we flip multikey path "a" to to true is not sufficient for protecting the above rollback scenario when wanting to flip multikey path "b". This is my understanding of what would still be a bug:
| |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Benety Goh [ 12/May/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
The atomic flags _isMultikeyForRead and _isMultikeyForWrite were added in
|