SERVER-43045 dbhash checks all replicated collections

Author: judahschvimer

jstests/replsets/dbhash_system_collections.js

@@ -32,6 +32,7 @@ function checkDbHash(mongo) {
 
     var replicatedAdminSystemCollections = [
         'system.backup_users',
+        'system.keys',
         'system.new_users',
         'system.roles',
         'system.version',

src/mongo/db/commands/dbhash.cpp

@@ -212,15 +212,6 @@ class DBHashCmd : public ErrmsgCommandDeprecated {
         md5_state_t globalState;
         md5_init(&globalState);
 
-        // A set of 'system' collections that are replicated, and therefore included in the db hash.
-        const std::set<StringData> replicatedSystemCollections{"system.backup_users",
-                                                               "system.js",
-                                                               "system.new_users",
-                                                               "system.roles",
-                                                               "system.users",
-                                                               "system.version",
-                                                               "system.views"};
-
         std::map<std::string, std::string> collectionToHashMap;
         std::map<std::string, OptionalCollectionUUID> collectionToUUIDMap;
         std::set<std::string> cappedCollectionSet;
@@ -235,11 +226,9 @@ class DBHashCmd : public ErrmsgCommandDeprecated {
                 return false;
             }
 
-            // Only include 'system' collections that are replicated.
-            bool isReplicatedSystemColl =
-                (replicatedSystemCollections.count(collNss.coll().toString()) > 0);
-            if (collNss.isSystem() && !isReplicatedSystemColl)
+            if (repl::ReplicationCoordinator::isOplogDisabledForNS(collNss)) {
                 return true;
+            }
 
             if (collNss.coll().startsWith("tmp.mr.")) {
                 // We skip any incremental map reduce collections as they also aren't

src/mongo/db/namespace_string.h

@@ -251,6 +251,12 @@ class NamespaceString {
      * Returns whether a namespace is replicated, based only on its string value. One notable
      * omission is that map reduce `tmp.mr` collections may or may not be replicated. Callers must
      * decide how to handle that case separately.
+     *
+     * Note: This function considers "replicated" to be any namespace that should be timestamped.
+     * Not all collections that are timestamped are replicated explicitly through the oplog.
+     * Drop-pending collections are a notable example. Please use
+     * ReplicationCoordinator::isOplogDisabledForNS to determine if a namespace gets logged in the
+     * oplog.
      */
     bool isReplicated() const;
 

src/mongo/db/repl/replication_coordinator.cpp

@@ -66,7 +66,7 @@ void ReplicationCoordinator::set(ServiceContext* service,
 }
 
 bool ReplicationCoordinator::isOplogDisabledFor(OperationContext* opCtx,
-                                                const NamespaceString& nss) {
+                                                const NamespaceString& nss) const {
     if (getReplicationMode() == ReplicationCoordinator::modeNone) {
         return true;
     }
@@ -75,7 +75,17 @@ bool ReplicationCoordinator::isOplogDisabledFor(OperationContext* opCtx,
         return true;
     }
 
-    if (nss.db() == "local") {
+    if (ReplicationCoordinator::isOplogDisabledForNS(nss)) {
+        return true;
+    }
+
+    fassert(28626, opCtx->recoveryUnit());
+
+    return false;
+}
+
+bool ReplicationCoordinator::isOplogDisabledForNS(const NamespaceString& nss) {
+    if (nss.isLocal()) {
         return true;
     }
 
@@ -87,8 +97,6 @@ bool ReplicationCoordinator::isOplogDisabledFor(OperationContext* opCtx,
         return true;
     }
 
-    fassert(28626, opCtx->recoveryUnit());
-
     return false;
 }
 

src/mongo/db/repl/replication_coordinator.h

@@ -890,10 +890,17 @@ class ReplicationCoordinator : public SyncSourceSelector {
     virtual void signalDropPendingCollectionsRemovedFromStorage() = 0;
 
     /**
-     * Returns true if logOp() should not append an entry to the oplog for the namespace for this
-     * operation.
+     * Returns true if logOp() should not append an entry to the oplog for this operation.
      */
-    bool isOplogDisabledFor(OperationContext* opCtx, const NamespaceString& nss);
+    bool isOplogDisabledFor(OperationContext* opCtx, const NamespaceString& nss) const;
+
+    /**
+     * Returns true if logOp() should never append an entry to the oplog for this namespace. logOp()
+     * may not want to append an entry to the oplog for other reasons, even if the namespace is
+     * allowed to be replicated in the oplog (e.g. being a secondary).
+     */
+    static bool isOplogDisabledForNS(const NamespaceString& nss);
+
 
     /**
      * Returns the stable timestamp that the storage engine recovered to on startup. If the

src/mongo/shell/replsettest.js

@@ -93,6 +93,7 @@ var ReplSetTest = function(opts) {
     var _allocatePortForBridge;
 
     var _causalConsistency;
+    var _isMultiVersion = false;
 
     // Some code still references kDefaultTimeoutMS as a (non-static) member variable, so make sure
     // it's still accessible that way.
@@ -1583,8 +1584,12 @@ var ReplSetTest = function(opts) {
                 // unreplicated "tmp.mr." collection may be left behind. We remove it from the
                 // dbHash command response to avoid an already known case of a mismatch.
                 // TODO SERVER-27147: Stop filtering out "tmp.mr." collections.
+                //
+                // TODO SERVER-43072: Stop filtering out 'system.keys' once 'last-stable' servers
+                // return 'system.keys' collections in dbhash responses (when 'last-stable' is 4.4).
                 if (cappedCollections.has(collName) ||
-                    (filterMapReduce && collName.startsWith("tmp.mr."))) {
+                    (filterMapReduce && collName.startsWith("tmp.mr.") ||
+                     (_isMultiVersion && collName === "system.keys"))) {
                     delete res.collections[collName];
                     // The "uuids" field in the dbHash command response is new as of MongoDB 4.0.
                     if (res.hasOwnProperty("uuids")) {
@@ -1979,7 +1984,11 @@ var ReplSetTest = function(opts) {
                         // If the primary and secondary have the same hashes for all the
                         // collections in the database and there aren't any capped collections,
                         // then the hashes for the whole database should match.
-                        if (primaryDBHash.md5 !== secondaryDBHash.md5) {
+
+                        // TODO SERVER-43072: Remove isMultiVersion exemption once 'last-stable'
+                        // servers return 'system.keys' collections in dbhash responses (when
+                        // 'last-stable' is 4.4).
+                        if (!_isMultiVersion && (primaryDBHash.md5 !== secondaryDBHash.md5)) {
                             print(msgPrefix +
                                   ', the primary and secondary have a different hash for ' +
                                   'the ' + dbName + ' database: ' + tojson(dbHashes));
@@ -2326,6 +2335,10 @@ var ReplSetTest = function(opts) {
             throw new Error("Failed to start node " + n);
         }
 
+        if (options.binVersion) {
+            _isMultiVersion = true;
+        }
+
         // Make sure to call _addPath, otherwise folders won't be cleaned.
         this._addPath(conn.dbpath);