TLDR; the MultiIndexBlock::_doCollectionScan runs a while-loop that iterates a collection cursor and inserts once document at a time into the index build bulk loaders. The write can throw a WCE, which calls abandonSnapshot, resetting the active cursor. The cursor must be saved and restored around WCE handling. We hopefully can minimize save/restore to only when this code is run to handle constraint violations writes, where the WCE is occurring, to save performance. Should probably check that there aren't any other write paths in there that can throw.
----------------------------------------------
Copied from the test failure diagnosis:
----------------------------------------------
The invariant stack trace goes through the MultiIndexBlock::_doCollectionScan() function that calls getNext here. The cursor is reading from the collection in a while-loop, and as each document is read it gets inserted into the index builders tables. The inserts go through the bulk loader, through the AbstractIndexAccessMethod ::BulkBuilderImpl::insert. The BulkBuilder has special handling for documents that violate index constraints, writing them to a side table. The constraints violations side table does a write here that can throw a WCE and retry. Throwing the WCE causes abandonSnapshot() to be called. Lastly, Louis and I think that, since the cursor is not saved and restored around the insert, the abandonSnapshot() call must reset the cursor internally in WT.
The hybrid index build logic doesn't do a save/restore cursor around the inserts, unlike the old index build code. This was a significant performance improvement, as saving and restoring the cursor is very costly. We clearly need some of the saves/restores back, but Louis suggested limiting those calls to only when writing to the constraints violations side table. It is not expected that there will be maybe violation constraints during an index build, except in pathological cases.
We could pass the cursor down to this logic that writes out to the side table – or somewhere lower, even. Louis suggested maybe passing down a lambda so as to avoid adding linking / code layer violation to the lower level code. Given the perf gains when the save/restore calls were removed with hybrid builds, we should probably sacrifice code niceties for performance. Other ideas might be passing flags down to recognize when the WCE+abandonSnapshot() has triggered, so we could save and use the high level record id before and after this call,, using the record id to forward the cursor back to where we need – that's what save/restore does, save the record ID on save and seek for the record ID on restore.
- causes
-
SERVER-60451 Index builds code can access an invalid BSONObj because the cursor was saved&restored at a lower level
- Closed