SERVER-21545 Correctly roll back updating collection options

4 files changed, 58 insertions, 7 deletions

diff --git a/jstests/core/collmod_bad_spec.js b/jstests/core/collmod_bad_spec.js
new file mode 100644
index 00000000000..43d7c43882d
--- /dev/null
+++ b/jstests/core/collmod_bad_spec.js
@@ -0,0 +1,23 @@
+// This is a regression test for SERVER-21545.
+//
+// Tests that a collMod with a bad specification does not cause any changes, and does not crash the
+// server.
+(function() {
+    "use strict";
+
+    var collName = "collModBadSpec";
+    var coll = db.getCollection(collName);
+
+    coll.drop();
+    assert.commandWorked(db.createCollection(collName));
+
+    // Get the original collection options for the collection.
+    var originalResult = db.getCollectionInfos({name: collName});
+
+    // Issue an invalid command.
+    assert.commandFailed(coll.runCommand("collMod", {usePowerOf2Sizes: true, unknownField: "x"}));
+
+    // Make sure the options are unchanged.
+    var newResult = db.getCollectionInfos({name: collName});
+    assert.eq(originalResult, newResult);
+})();
diff --git a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp
index 0c70f9959ea..d8b32fbe47c 100644
--- a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp
+++ b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp
@@ -39,6 +39,7 @@
 #include "mongo/db/storage/mmap_v1/catalog/namespace_details_rsv1_metadata.h"
 #include "mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.h"
 #include "mongo/db/storage/record_store.h"
+#include "mongo/db/storage/recovery_unit.h"
 #include "mongo/util/log.h"
 #include "mongo/util/startup_test.h"
 
@@ -58,7 +59,7 @@ NamespaceDetailsCollectionCatalogEntry::NamespaceDetailsCollectionCatalogEntry(
       _namespacesRecordStore(namespacesRecordStore),
       _indexRecordStore(indexRecordStore),
       _db(db) {
-    setNamespacesRecordId(namespacesRecordId);
+    setNamespacesRecordId(nullptr, namespacesRecordId);
 }
 
 CollectionOptions NamespaceDetailsCollectionCatalogEntry::getCollectionOptions(
@@ -363,7 +364,7 @@ void NamespaceDetailsCollectionCatalogEntry::_updateSystemNamespaces(OperationCo
     StatusWith<RecordId> result = _namespacesRecordStore->updateRecord(
         txn, _namespacesRecordId, newEntry.objdata(), newEntry.objsize(), false, NULL);
     fassert(17486, result.getStatus());
-    _namespacesRecordId = result.getValue();
+    setNamespacesRecordId(txn, result.getValue());
 }
 
 void NamespaceDetailsCollectionCatalogEntry::updateFlags(OperationContext* txn, int newValue) {
@@ -372,13 +373,37 @@ void NamespaceDetailsCollectionCatalogEntry::updateFlags(OperationContext* txn,
     _updateSystemNamespaces(txn, BSON("$set" << BSON("options.flags" << newValue)));
 }
 
-void NamespaceDetailsCollectionCatalogEntry::setNamespacesRecordId(RecordId newId) {
+namespace {
+class NamespacesRecordIdChange : public RecoveryUnit::Change {
+public:
+    NamespacesRecordIdChange(RecordId* namespacesRecordId, const RecordId& newId)
+        : _namespacesRecordId(namespacesRecordId), _oldId(*namespacesRecordId) {}
+
+    void commit() override {}
+    void rollback() override {
+        *_namespacesRecordId = _oldId;
+    }
+
+private:
+    RecordId* _namespacesRecordId;
+    const RecordId _oldId;
+};
+}
+
+void NamespaceDetailsCollectionCatalogEntry::setNamespacesRecordId(OperationContext* txn,
+                                                                   RecordId newId) {
     if (newId.isNull()) {
         invariant(ns().coll() == "system.namespaces" || ns().coll() == "system.indexes");
     } else {
-        // We don't need an OperationContext in MMAP, so 'nullptr' is OK.
-        auto namespaceEntry = _namespacesRecordStore->dataFor(nullptr, newId).releaseToBson();
+        // 'txn' is allowed to be null, but we don't need an OperationContext in MMAP, so that's OK.
+        auto namespaceEntry = _namespacesRecordStore->dataFor(txn, newId).releaseToBson();
         invariant(namespaceEntry["name"].String() == ns().ns());
+
+        // Register RecordId change for rollback if we're not initializing.
+        if (txn && !_namespacesRecordId.isNull()) {
+            txn->recoveryUnit()->registerChange(
+                new NamespacesRecordIdChange(&_namespacesRecordId, newId));
+        }
         _namespacesRecordId = newId;
     }
 }
diff --git a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h
index 2128fda14cd..44b7bfb6051 100644
--- a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h
+++ b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h
@@ -103,7 +103,10 @@ public:
         return _namespacesRecordId;
     }
 
-    void setNamespacesRecordId(RecordId newId);
+    /**
+     * 'txn' is only allowed to be null when called from the constructor.
+     */
+    void setNamespacesRecordId(OperationContext* txn, RecordId newId);
 
 private:
     NamespaceDetails* _details;
diff --git a/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp b/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp
index 3a5ada58046..d7c0c4fe8e9 100644
--- a/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp
+++ b/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp
@@ -574,7 +574,7 @@ void MMAPV1DatabaseCatalogEntry::_init(OperationContext* txn) {
         // because they don't have indexes on them anyway.
         if (entry) {
             if (entry->catalogEntry->getNamespacesRecordId().isNull()) {
-                entry->catalogEntry->setNamespacesRecordId(rid);
+                entry->catalogEntry->setNamespacesRecordId(txn, rid);
             } else {
                 invariant(entry->catalogEntry->getNamespacesRecordId() == rid);
             }