SERVER-47694: fix multikey. again

Split the single _isMultikey variable on an IndexCatalogEntry(Impl) into two
separate variables: _isMultikeyForReader and _isMultikeyForWriter.

_isMultikeyForReader is flipped as early as possible. Readers concurrent
with multikey flipping may forgo a possible optimization when their snapshot
sees no multikey data.

_isMultikeyForWriter is flipped after the storage engine commits a multikey
change to the on-disk catalog. At this point, writers may, under some
circumstances, optimize away some catalog writes.

Move logic for optimizing readers (multikey paths, clearing query cache) outside
of the onCommit.

Adds a failpoint widenWUOWChangesWindow which sleeps transaction commit and
onCommit/onRollback handlers.

Have validate assert multikey paths are set correctly for the documents observed
during its collection scan.

1 files changed, 33 insertions, 26 deletions

diff --git a/src/mongo/db/catalog/index_catalog_entry_impl.cpp b/src/mongo/db/catalog/index_catalog_entry_impl.cpp
index 832d3d2767d..cca329b0007 100644
--- a/src/mongo/db/catalog/index_catalog_entry_impl.cpp
+++ b/src/mongo/db/catalog/index_catalog_entry_impl.cpp
@@ -80,7 +80,9 @@ IndexCatalogEntryImpl::IndexCatalogEntryImpl(OperationContext* const opCtx,
 
     {
         stdx::lock_guard<Latch> lk(_indexMultikeyPathsMutex);
-        _isMultikey.store(_catalogIsMultikey(opCtx, &_indexMultikeyPaths));
+        const bool isMultikey = _catalogIsMultikey(opCtx, &_indexMultikeyPaths);
+        _isMultikeyForRead.store(isMultikey);
+        _isMultikeyForWrite.store(isMultikey);
         _indexTracksPathLevelMultikeyInfo = !_indexMultikeyPaths.empty();
     }
 
@@ -158,7 +160,7 @@ bool IndexCatalogEntryImpl::isFrozen() const {
 }
 
 bool IndexCatalogEntryImpl::isMultikey() const {
-    return _isMultikey.load();
+    return _isMultikeyForRead.load();
 }
 
 MultikeyPaths IndexCatalogEntryImpl::getMultikeyPaths(OperationContext* opCtx) const {
@@ -180,7 +182,7 @@ void IndexCatalogEntryImpl::setIsReady(bool newIsReady) {
 
 void IndexCatalogEntryImpl::setMultikey(OperationContext* opCtx,
                                         const MultikeyPaths& multikeyPaths) {
-    if (!_indexTracksPathLevelMultikeyInfo && isMultikey()) {
+    if (!_indexTracksPathLevelMultikeyInfo && _isMultikeyForWrite.load()) {
         // If the index is already set as multikey and we don't have any path-level information to
         // update, then there's nothing more for us to do.
         return;
@@ -335,30 +337,35 @@ void IndexCatalogEntryImpl::_catalogSetMultikey(OperationContext* opCtx,
                                                        _descriptor->indexName(),
                                                        multikeyPaths);
 
-    // The commit handler for a transaction that sets the multikey flag. When the recovery unit
-    // commits, update the multikey paths if needed and clear the plan cache if the index metadata
-    // has changed.
-    opCtx->recoveryUnit()->onCommit(
-        [this, multikeyPaths, indexMetadataHasChanged](boost::optional<Timestamp>) {
-            _isMultikey.store(true);
-
-            if (_indexTracksPathLevelMultikeyInfo) {
-                stdx::lock_guard<Latch> lk(_indexMultikeyPathsMutex);
-                for (size_t i = 0; i < multikeyPaths.size(); ++i) {
-                    _indexMultikeyPaths[i].insert(multikeyPaths[i].begin(), multikeyPaths[i].end());
-                }
-            }
-
-            if (indexMetadataHasChanged && _queryInfo) {
-                LOGV2_DEBUG(
-                    20351,
+    // In the absense of using the storage engine to read from the catalog, we must set multikey
+    // prior to the storage engine transaction committing.
+    //
+    // Moreover, there must not be an `onRollback` handler to reset this back to false. Given a long
+    // enough pause in processing `onRollback` handlers, a later writer that successfully flipped
+    // multikey can be undone. Alternatively, one could use a counter instead of a boolean to avoid
+    // that problem.
+    _isMultikeyForRead.store(true);
+    if (_indexTracksPathLevelMultikeyInfo) {
+        stdx::lock_guard<Latch> lk(_indexMultikeyPathsMutex);
+        for (size_t i = 0; i < multikeyPaths.size(); ++i) {
+            _indexMultikeyPaths[i].insert(multikeyPaths[i].begin(), multikeyPaths[i].end());
+        }
+    }
+    if (indexMetadataHasChanged && _queryInfo) {
+        LOGV2_DEBUG(47187005,
                     1,
-                    "{ns}: clearing plan cache - index {descriptor_keyPattern} set to multi key.",
-                    "ns"_attr = ns(),
-                    "descriptor_keyPattern"_attr = _descriptor->keyPattern());
-                _queryInfo->clearQueryCache();
-            }
-        });
+                    "Index set to multi key, clearing query plan cache",
+                    "namespace"_attr = ns(),
+                    "keyPattern"_attr = _descriptor->keyPattern());
+        _queryInfo->clearQueryCache();
+    }
+
+    opCtx->recoveryUnit()->onCommit([this](boost::optional<Timestamp>) {
+        // Writers must attempt to flip multikey until it's confirmed a storage engine
+        // transaction successfully commits. Only after this point may a writer optimize out
+        // flipping multikey.
+        _isMultikeyForWrite.store(true);
+    });
 }
 
 KVPrefix IndexCatalogEntryImpl::_catalogGetPrefix(OperationContext* opCtx) const {