[SERVER-34556] Iterating through indexes when reading from a timestamp behind the minimumVisibleSnapshot should not cause an invariant Created: 18/Apr/18 Updated: 29/Oct/23 Resolved: 17/May/18 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Storage |
| Affects Version/s: | 3.7.5 |
| Fix Version/s: | 4.0.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Louis Williams | Assignee: | Dianna Hohensee (Inactive) |
| 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 | ||||
| Sprint: | Storage NYC 2018-05-07, Storage NYC 2018-05-21 | ||||
| Participants: | |||||
| Linked BF Score: | 16 | ||||
| Description |
|
An invariant is hit when an index exists in memory (because it has completed), but it is not yet visible on the storage engine when reading at an earlier timestamp. A query that reads from a timestamp is allowed to yield. When its locks are reacquired and it reads again, say it reads from a newer timestamp, T1. If an index build occurs during the yield at T2, and the read timestamp on the query is before the index build timestamp, iterating through the list of indexes on a collection can trigger this invariant. Rather than always triggering an invariant, we should just not show the index as ready. This will only be the case when the current operation is reading from a timestamp and the index is marked ready in memory but not visible on the storage engine. |
| Comments |
| Comment by Githook User [ 17/May/18 ] |
|
Author: {'email': 'dianna.hohensee@10gen.com', 'username': 'DiannaHohensee', 'name': 'Dianna Hohensee'}Message: |
| Comment by Dianna Hohensee (Inactive) [ 17/May/18 ] |
|
Thanks for the pointers, daniel.gottlieb, louis.williams! In conclusion, it appears for kLastApplied reads that this check would return false and the code would continue on to hit the invariant. This is because getPointInTimeReadTimestamp returns boost::none for kLastApplied. Th invariant is hit because IndexCatalogEntryImpl is unaware of timestamps and depends upon IndexCatalogImpl::IndexIteratorImpl to manage timestamps such that it (IndexCatalogEntryImpl) never must. In this case IndexCatalogImpl::IndexIteratorImpl did not manage it correctly. |
| Comment by Louis Williams [ 15/May/18 ] |
|
After
|
| Comment by Daniel Gottlieb (Inactive) [ 14/May/18 ] |
|
Perhaps an alternative perspective, this condition to prevent from reading behind the `minSnapshot` is ineffective because the recovery unit does not acknowledge it is reading from a point in time when the read concern level is local (which is the default). Moreover, (in the case of reading from last applied) the _readAtTimestamp/_majorityCommittedSnapshot are the null timestamp. It seems the problem could be resolved, if instead the iterating code could reliably know the timestamp that is being read at. In general it seems like a much more useful contract to transparently return the read time. |
| Comment by Dianna Hohensee (Inactive) [ 27/Apr/18 ] |
|
Talked with Louis and he believes that a recent commit of his incidentally fixed this bug. The new code now advances the minVisibleSnapshot on the in-memory catalog entry when the index is committed to the storage engine. This should avoid the invariant check performed in this call because the code should now return true here and never the invariant. The minVisibleSnapshot of the index is now greater than the reader's timestamp, and the index is ignored correctly. The invariant was originally getting tripped because this code ensures in-memory and storage engine both believe either that the index is ready or not ready. With a read started on a timestamp predating the index build, the storage engine would obey that timestamp and say that the index does not exist; whereas the in-memory answer to whether the index is ready is not versioned, and reports yes to ready. |