SERVER-41766 Update the on-disk multikey flag in a side transaction when running inside a multi-document transaction

When a write inside a multi-document transaction needs to set an index as multikey, we update the multikey flag in the on-disk catalog in a transaction separate from the parent transaction. We commit this side transaction immediately, so as to avoid the catalog write generating prepare conflicts if it was written as part of a parent transaction that later became prepared. In general, it is safe to set an index as multikey too early. The multikey write is timestamped at the most recent value of the LogicalClock.

2 files changed, 143 insertions, 20 deletions

diff --git a/src/mongo/db/catalog/index_catalog_entry_impl.cpp b/src/mongo/db/catalog/index_catalog_entry_impl.cpp
index d3d7c79a5a7..6736d545453 100644
--- a/src/mongo/db/catalog/index_catalog_entry_impl.cpp
+++ b/src/mongo/db/catalog/index_catalog_entry_impl.cpp
@@ -43,6 +43,7 @@
 #include "mongo/db/concurrency/write_conflict_exception.h"
 #include "mongo/db/index/index_access_method.h"
 #include "mongo/db/index/index_descriptor.h"
+#include "mongo/db/logical_clock.h"
 #include "mongo/db/matcher/expression.h"
 #include "mongo/db/matcher/expression_parser.h"
 #include "mongo/db/multi_key_path_tracker.h"
@@ -267,31 +268,68 @@ void IndexCatalogEntryImpl::setMultikey(OperationContext* opCtx,
     // CollectionCatalogEntry::setIndexIsMultikey() requires that we discard the path-level
     // multikey information in order to avoid unintentionally setting path-level multikey
     // information on an index created before 3.4.
-    const bool indexMetadataHasChanged = DurableCatalog::get(opCtx)->setIndexIsMultikey(
-        opCtx, _collection->ns(), _descriptor->indexName(), paths);
-
-    // 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<stdx::mutex> lk(_indexMultikeyPathsMutex);
-                for (size_t i = 0; i < multikeyPaths.size(); ++i) {
-                    _indexMultikeyPaths[i].insert(multikeyPaths[i].begin(), multikeyPaths[i].end());
-                }
+    bool indexMetadataHasChanged;
+
+    // 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.
+    auto onMultikeyCommitFn = [this, multikeyPaths](bool indexMetadataHasChanged) {
+        _isMultikey.store(true);
+
+        if (_indexTracksPathLevelMultikeyInfo) {
+            stdx::lock_guard<stdx::mutex> lk(_indexMultikeyPathsMutex);
+            for (size_t i = 0; i < multikeyPaths.size(); ++i) {
+                _indexMultikeyPaths[i].insert(multikeyPaths[i].begin(), multikeyPaths[i].end());
             }
+        }
 
-            if (indexMetadataHasChanged && _infoCache) {
-                LOG(1) << _ns << ": clearing plan cache - index " << _descriptor->keyPattern()
-                       << " set to multi key.";
-                _infoCache->clearQueryCache();
+        if (indexMetadataHasChanged && _infoCache) {
+            LOG(1) << _ns << ": clearing plan cache - index " << _descriptor->keyPattern()
+                   << " set to multi key.";
+            _infoCache->clearQueryCache();
+        }
+    };
+
+    // If we are inside a multi-document transaction, we write the on-disk multikey update in a
+    // separate transaction so that it will not generate prepare conflicts with other operations
+    // that try to set the multikey flag. In general, it should always be safe to update the
+    // multikey flag earlier than necessary, and so we are not concerned with the atomicity of the
+    // multikey flag write and the parent transaction. We can do this write separately and commit it
+    // before the parent transaction commits.
+    auto txnParticipant = TransactionParticipant::get(opCtx);
+    if (txnParticipant && txnParticipant.inMultiDocumentTransaction()) {
+        TransactionParticipant::SideTransactionBlock sideTxn(opCtx);
+        writeConflictRetry(opCtx, "set index multikey", _collection->ns().ns(), [&] {
+            WriteUnitOfWork wuow(opCtx);
+            auto writeTs = LogicalClock::get(opCtx)->getClusterTime().asTimestamp();
+            auto status = opCtx->recoveryUnit()->setTimestamp(writeTs);
+            if (status.code() == ErrorCodes::BadValue) {
+                log() << "Temporarily could not timestamp the multikey catalog write, retrying. "
+                      << status.reason();
+                throw WriteConflictException();
             }
+            fassert(31164, status);
+            indexMetadataHasChanged = DurableCatalog::get(opCtx)->setIndexIsMultikey(
+                opCtx, _collection->ns(), _descriptor->indexName(), paths);
+            opCtx->recoveryUnit()->onCommit([onMultikeyCommitFn, indexMetadataHasChanged](
+                boost::optional<Timestamp>) { onMultikeyCommitFn(indexMetadataHasChanged); });
+            wuow.commit();
         });
+    } else {
+        indexMetadataHasChanged = DurableCatalog::get(opCtx)->setIndexIsMultikey(
+            opCtx, _collection->ns(), _descriptor->indexName(), paths);
+    }
 
-    // Keep multikey changes in memory to correctly service later reads using this index.
-    auto txnParticipant = TransactionParticipant::get(opCtx);
+    opCtx->recoveryUnit()->onCommit([onMultikeyCommitFn, indexMetadataHasChanged](
+        boost::optional<Timestamp>) { onMultikeyCommitFn(indexMetadataHasChanged); });
+
+    // Within a multi-document transaction, reads should be able to see the effect of previous
+    // writes done within that transaction. If a previous write in a transaction has set the index
+    // to be multikey, then a subsequent read MUST know that fact in order to return correct
+    // results. This is true in general for multikey writes. Since we don't update the in-memory
+    // multikey flag until after the transaction commits, we track extra information here to let
+    // subsequent readers within the same transaction know if this index was set as multikey by a
+    // previous write in the transaction.
     if (txnParticipant && txnParticipant.inMultiDocumentTransaction()) {
         txnParticipant.addUncommittedMultikeyPathInfo(
             MultikeyPathInfo{_collection->ns(), _descriptor->indexName(), std::move(paths)});
diff --git a/src/mongo/dbtests/storage_timestamp_tests.cpp b/src/mongo/dbtests/storage_timestamp_tests.cpp
index 5cd50ea0088..2b48c91607d 100644
--- a/src/mongo/dbtests/storage_timestamp_tests.cpp
+++ b/src/mongo/dbtests/storage_timestamp_tests.cpp
@@ -1587,6 +1587,90 @@ public:
     }
 };
 
+class PrimarySetsMultikeyInsideMultiDocumentTransaction : public StorageTimestampTest {
+
+public:
+    void run() {
+        auto service = _opCtx->getServiceContext();
+        auto sessionCatalog = SessionCatalog::get(service);
+        sessionCatalog->reset_forTest();
+        MongoDSessionCatalog::onStepUp(_opCtx);
+
+        NamespaceString nss("unittests.PrimarySetsMultikeyInsideMultiDocumentTransaction");
+        reset(nss);
+
+        auto indexName = "a_1";
+        auto indexSpec =
+            BSON("name" << indexName << "ns" << nss.ns() << "key" << BSON("a" << 1) << "v"
+                        << static_cast<int>(kIndexVersion));
+        auto doc = BSON("_id" << 1 << "a" << BSON_ARRAY(1 << 2));
+
+        {
+            AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_IX);
+            ASSERT_OK(dbtests::createIndexFromSpec(_opCtx, nss.ns(), indexSpec));
+        }
+
+        auto presentTs = _clock->getClusterTime().asTimestamp();
+
+        // This test does not run a real ReplicationCoordinator, so must advance the snapshot
+        // manager manually.
+        auto storageEngine = cc().getServiceContext()->getStorageEngine();
+        storageEngine->getSnapshotManager()->setLocalSnapshot(presentTs);
+
+        const auto beforeTxnTime = _clock->reserveTicks(1);
+        auto beforeTxnTs = beforeTxnTime.asTimestamp();
+        auto commitEntryTs = beforeTxnTime.addTicks(1).asTimestamp();
+
+        unittest::log() << "Present TS: " << presentTs;
+        unittest::log() << "Before transaction TS: " << beforeTxnTs;
+        unittest::log() << "Commit entry TS: " << commitEntryTs;
+
+        const auto sessionId = makeLogicalSessionIdForTest();
+        _opCtx->setLogicalSessionId(sessionId);
+        _opCtx->setTxnNumber(1);
+
+        // Check out the session.
+        MongoDOperationContextSession ocs(_opCtx);
+
+        auto txnParticipant = TransactionParticipant::get(_opCtx);
+        ASSERT(txnParticipant);
+
+        txnParticipant.beginOrContinue(
+            _opCtx, *_opCtx->getTxnNumber(), false /* autocommit */, true /* startTransaction */);
+        txnParticipant.unstashTransactionResources(_opCtx, "insert");
+        {
+            // Insert a document that will set the index as multikey.
+            AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_IX);
+            insertDocument(autoColl.getCollection(), InsertStatement(doc));
+        }
+
+        txnParticipant.commitUnpreparedTransaction(_opCtx);
+        txnParticipant.stashTransactionResources(_opCtx);
+
+        AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_IX);
+        auto coll = autoColl.getCollection();
+
+        // Make sure the transaction committed and its writes were timestamped correctly.
+        assertDocumentAtTimestamp(coll, beforeTxnTs, BSONObj());
+        assertDocumentAtTimestamp(coll, presentTs, BSONObj());
+        assertDocumentAtTimestamp(coll, commitEntryTs, doc);
+        assertDocumentAtTimestamp(coll, nullTs, doc);
+
+        // Make sure the multikey write was timestamped correctly. For correctness, the timestamp of
+        // the write that sets the multikey flag to true should be less than or equal to the first
+        // write that made the index multikey, which, in this case, is the commit timestamp of the
+        // transaction. In other words, it is not incorrect to assign a timestamp that is too early,
+        // but it is incorrect to assign a timestamp that is too late. In this specific case, we
+        // expect the write to be timestamped at the logical clock tick directly preceding the
+        // commit time.
+        assertMultikeyPaths(_opCtx, coll, indexName, presentTs, false /* shouldBeMultikey */, {{}});
+        assertMultikeyPaths(
+            _opCtx, coll, indexName, beforeTxnTs, true /* shouldBeMultikey */, {{0}});
+        assertMultikeyPaths(
+            _opCtx, coll, indexName, commitEntryTs, true /* shouldBeMultikey */, {{0}});
+    }
+};
+
 class InitializeMinValid : public StorageTimestampTest {
 public:
     void run() {
@@ -3503,6 +3587,7 @@ public:
         add<InitialSyncSetIndexMultikeyOnInsert>();
         add<PrimarySetIndexMultikeyOnInsert>();
         add<PrimarySetIndexMultikeyOnInsertUnreplicated>();
+        add<PrimarySetsMultikeyInsideMultiDocumentTransaction>();
         add<InitializeMinValid>();
         add<SetMinValidInitialSyncFlag>();
         add<SetMinValidToAtLeast>();