SERVER-30106 Allow syncTail replication to write to drop-pending collections

Reinstated after revert in a1c67941bf08c69cab04eba20bc9ce9a763e1c7f

8 files changed, 56 insertions, 71 deletions

diff --git a/src/mongo/db/cloner.cpp b/src/mongo/db/cloner.cpp
index 42081c94b79..7c26f24ed88 100644
--- a/src/mongo/db/cloner.cpp
+++ b/src/mongo/db/cloner.cpp
@@ -539,7 +539,7 @@ StatusWith<std::vector<BSONObj>> Cloner::filterCollectionsForClone(
         const NamespaceString ns(opts.fromDB, collectionName.c_str());
 
         if (ns.isSystem()) {
-            if (legalClientSystemNS(ns.ns()) == 0) {
+            if (!ns.isLegalClientSystemNS()) {
                 LOG(2) << "\t\t not cloning because system collection";
                 continue;
             }
diff --git a/src/mongo/db/concurrency/lock_state.cpp b/src/mongo/db/concurrency/lock_state.cpp
index 710de4ac234..721067e3aad 100644
--- a/src/mongo/db/concurrency/lock_state.cpp
+++ b/src/mongo/db/concurrency/lock_state.cpp
@@ -505,6 +505,8 @@ bool LockerImpl<IsForMMAPV1>::isCollectionLockedForMode(StringData ns, LockMode
     const ResourceId resIdDb(RESOURCE_DATABASE, nss.db());
 
     LockMode dbMode = getLockMode(resIdDb);
+    if (!shouldConflictWithSecondaryBatchApplication())
+        return true;
 
     switch (dbMode) {
         case MODE_NONE:
diff --git a/src/mongo/db/namespace_string.cpp b/src/mongo/db/namespace_string.cpp
index b9cedd47a95..eaf1fab39ff 100644
--- a/src/mongo/db/namespace_string.cpp
+++ b/src/mongo/db/namespace_string.cpp
@@ -79,33 +79,6 @@ constexpr auto dropPendingNSPrefix = "system.drop."_sd;
 
 }  // namespace
 
-bool legalClientSystemNS(StringData ns) {
-    if (ns == "local.system.replset")
-        return true;
-
-    if (ns.find(".system.users") != string::npos)
-        return true;
-
-    if (ns == "admin.system.roles")
-        return true;
-    if (ns == kServerConfiguration)
-        return true;
-    if (ns == kLogicalTimeKeysCollection)
-        return true;
-    if (ns == "admin.system.new_users")
-        return true;
-    if (ns == "admin.system.backup_users")
-        return true;
-
-    if (ns.find(".system.js") != string::npos)
-        return true;
-
-    if (nsToCollectionSubstring(ns) == NamespaceString::kSystemDotViewsCollectionName)
-        return true;
-
-    return false;
-}
-
 constexpr StringData NamespaceString::kAdminDb;
 constexpr StringData NamespaceString::kLocalDb;
 constexpr StringData NamespaceString::kConfigDb;
@@ -129,6 +102,33 @@ bool NamespaceString::isCollectionlessAggregateNS() const {
     return coll() == collectionlessAggregateCursorCol;
 }
 
+bool NamespaceString::isLegalClientSystemNS() const {
+    if (db() == "admin") {
+        if (ns() == "admin.system.roles")
+            return true;
+        if (ns() == kServerConfiguration)
+            return true;
+        if (ns() == kLogicalTimeKeysCollection)
+            return true;
+        if (ns() == "admin.system.new_users")
+            return true;
+        if (ns() == "admin.system.backup_users")
+            return true;
+    }
+    if (ns() == "local.system.replset")
+        return true;
+
+    if (coll() == "system.users")
+        return true;
+    if (coll() == "system.js")
+        return true;
+
+    if (coll() == kSystemDotViewsCollectionName)
+        return true;
+
+    return false;
+}
+
 NamespaceString NamespaceString::makeListCollectionsNSS(StringData dbName) {
     NamespaceString nss(dbName, listCollectionsCursorCol);
     dassert(nss.isValid());
diff --git a/src/mongo/db/namespace_string.h b/src/mongo/db/namespace_string.h
index 24abea86771..5572eaab886 100644
--- a/src/mongo/db/namespace_string.h
+++ b/src/mongo/db/namespace_string.h
@@ -43,11 +43,6 @@ namespace mongo {
 
 const size_t MaxDatabaseNameLen = 128;  // max str len for the db name, including null char
 
-/** @return true if a client can modify this namespace even though it is under ".system."
-    For example <dbname>.system.users is ok for regular clients to update.
-*/
-bool legalClientSystemNS(StringData ns);
-
 /* e.g.
    NamespaceString ns("acme.orders");
    cout << ns.coll; // "orders"
@@ -227,6 +222,12 @@ public:
     bool isListIndexesCursorNS() const;
 
     /**
+     * Returns true if a client can modify this namespace even though it is under ".system."
+     * For example <dbname>.system.users is ok for regular clients to update.
+     */
+    bool isLegalClientSystemNS() const;
+
+    /**
      * Given a NamespaceString for which isGloballyManagedNamespace() returns true, returns the
      * namespace the command targets, or boost::none for commands like 'listCollections' which
      * do not target a collection.
diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp
index 5367ce9c82b..bb2d6a6b68e 100644
--- a/src/mongo/db/query/get_executor.cpp
+++ b/src/mongo/db/query/get_executor.cpp
@@ -696,8 +696,8 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorDelete(
 
     const NamespaceString& nss(request->getNamespaceString());
     if (!request->isGod()) {
-        if (nss.isSystem()) {
-            uassert(12050, "cannot delete from system namespace", legalClientSystemNS(nss.ns()));
+        if (nss.isSystem() && opCtx->lockState()->shouldConflictWithSecondaryBatchApplication()) {
+            uassert(12050, "cannot delete from system namespace", nss.isLegalClientSystemNS());
         }
         if (nss.isVirtualized()) {
             log() << "cannot delete from a virtual collection: " << nss;
@@ -824,33 +824,21 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorDelete(
 // Update
 //
 
-namespace {
-
-// TODO: Make this a function on NamespaceString, or make it cleaner.
-inline void validateUpdate(const char* ns, const BSONObj& updateobj, const BSONObj& patternOrig) {
-    uassert(10155, "cannot update reserved $ collection", strchr(ns, '$') == 0);
-    if (strstr(ns, ".system.")) {
-        /* dm: it's very important that system.indexes is never updated as IndexDetails
-           has pointers into it */
-        uassert(10156,
-                str::stream() << "cannot update system collection: " << ns << " q: " << patternOrig
-                              << " u: "
-                              << updateobj,
-                legalClientSystemNS(ns));
-    }
-}
-
-}  // namespace
-
 StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
     OperationContext* opCtx, OpDebug* opDebug, Collection* collection, ParsedUpdate* parsedUpdate) {
     const UpdateRequest* request = parsedUpdate->getRequest();
     UpdateDriver* driver = parsedUpdate->getDriver();
 
-    const NamespaceString& nsString = request->getNamespaceString();
+    const NamespaceString& nss = request->getNamespaceString();
     UpdateLifecycle* lifecycle = request->getLifecycle();
 
-    validateUpdate(nsString.ns().c_str(), request->getUpdates(), request->getQuery());
+    if (nss.isSystem() && opCtx->lockState()->shouldConflictWithSecondaryBatchApplication()) {
+        uassert(10156, "cannot update a system namespace", nss.isLegalClientSystemNS());
+    }
+    if (nss.isVirtualized()) {
+        log() << "cannot update a virtual collection: " << nss;
+        uasserted(10155, "cannot update a virtual collection");
+    }
 
     // If there is no collection and this is an upsert, callers are supposed to create
     // the collection prior to calling this method. Explain, however, will never do
@@ -870,11 +858,11 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
     // writes on a secondary. If this is an update to a secondary from the replication system,
     // however, then we make an exception and let the write proceed.
     bool userInitiatedWritesAndNotPrimary = opCtx->writesAreReplicated() &&
-        !repl::getGlobalReplicationCoordinator()->canAcceptWritesFor(opCtx, nsString);
+        !repl::getGlobalReplicationCoordinator()->canAcceptWritesFor(opCtx, nss);
 
     if (userInitiatedWritesAndNotPrimary) {
         return Status(ErrorCodes::PrimarySteppedDown,
-                      str::stream() << "Not primary while performing update on " << nsString.ns());
+                      str::stream() << "Not primary while performing update on " << nss.ns());
     }
 
     if (lifecycle) {
@@ -896,12 +884,11 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
             // Treat collections that do not exist as empty collections. Note that the explain
             // reporting machinery always assumes that the root stage for an update operation is
             // an UpdateStage, so in this case we put an UpdateStage on top of an EOFStage.
-            LOG(2) << "Collection " << nsString.ns() << " does not exist."
+            LOG(2) << "Collection " << nss.ns() << " does not exist."
                    << " Using EOF stage: " << redact(unparsedQuery);
             auto updateStage = make_unique<UpdateStage>(
                 opCtx, updateStageParams, ws.get(), collection, new EOFStage(opCtx));
-            return PlanExecutor::make(
-                opCtx, std::move(ws), std::move(updateStage), nsString, policy);
+            return PlanExecutor::make(opCtx, std::move(ws), std::move(updateStage), nss, policy);
         }
 
         const IndexDescriptor* descriptor = collection->getIndexCatalog()->findIdIndex(opCtx);
@@ -957,13 +944,8 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
         // is invalid to use a positional projection because the query expression need not
         // match the array element after the update has been applied.
         const bool allowPositional = request->shouldReturnOldDocs();
-        StatusWith<unique_ptr<PlanStage>> projStatus = applyProjection(opCtx,
-                                                                       nsString,
-                                                                       cq.get(),
-                                                                       request->getProj(),
-                                                                       allowPositional,
-                                                                       ws.get(),
-                                                                       std::move(root));
+        StatusWith<unique_ptr<PlanStage>> projStatus = applyProjection(
+            opCtx, nss, cq.get(), request->getProj(), allowPositional, ws.get(), std::move(root));
         if (!projStatus.isOK()) {
             return projStatus.getStatus();
         }
diff --git a/src/mongo/db/repl/databases_cloner.cpp b/src/mongo/db/repl/databases_cloner.cpp
index 74c4543da21..d03f7252c8b 100644
--- a/src/mongo/db/repl/databases_cloner.cpp
+++ b/src/mongo/db/repl/databases_cloner.cpp
@@ -321,7 +321,7 @@ void DatabasesCloner::_onListDatabaseFinish(const CommandCallbackArgs& cbd) {
         const auto collectionFilterPred = [dbName](const BSONObj& collInfo) {
             const auto collName = collInfo["name"].str();
             const NamespaceString ns(dbName, collName);
-            if (ns.isSystem() && !legalClientSystemNS(ns.ns())) {
+            if (ns.isSystem() && !ns.isLegalClientSystemNS()) {
                 LOG(1) << "Skipping 'system' collection: " << ns.ns();
                 return false;
             }
diff --git a/src/mongo/db/repl/storage_interface_impl.cpp b/src/mongo/db/repl/storage_interface_impl.cpp
index 0dcb15a0110..672a2eb16ea 100644
--- a/src/mongo/db/repl/storage_interface_impl.cpp
+++ b/src/mongo/db/repl/storage_interface_impl.cpp
@@ -830,7 +830,7 @@ Status StorageInterfaceImpl::deleteByFilter(OperationContext* opCtx,
     request.setMulti(true);
     request.setYieldPolicy(PlanExecutor::NO_YIELD);
 
-    // This disables the legalClientSystemNS() check in getExecutorDelete() which is used to
+    // This disables the isLegalClientSystemNS() check in getExecutorDelete() which is used to
     // disallow client deletes from unrecognized system collections.
     request.setGod();
 
diff --git a/src/mongo/db/repl/storage_interface_impl_test.cpp b/src/mongo/db/repl/storage_interface_impl_test.cpp
index 70e801e2fe3..825402ef879 100644
--- a/src/mongo/db/repl/storage_interface_impl_test.cpp
+++ b/src/mongo/db/repl/storage_interface_impl_test.cpp
@@ -1844,7 +1844,7 @@ TEST_F(StorageInterfaceImplTest,
     // Checks that we can update collections with namespaces not considered "legal client system"
     // namespaces.
     NamespaceString nss("local.system.rollback.docs");
-    ASSERT_FALSE(legalClientSystemNS(nss.ns()));
+    ASSERT_FALSE(nss.isLegalClientSystemNS());
 
     auto opCtx = getOperationContext();
     StorageInterfaceImpl storage;
@@ -2088,7 +2088,7 @@ TEST_F(StorageInterfaceImplTest, DeleteByFilterRemovesDocumentsInIllegalClientSy
     // Checks that we can remove documents from collections with namespaces not considered "legal
     // client system" namespaces.
     NamespaceString nss("local.system.rollback.docs");
-    ASSERT_FALSE(legalClientSystemNS(nss.ns()));
+    ASSERT_FALSE(nss.isLegalClientSystemNS());
 
     auto opCtx = getOperationContext();
     StorageInterfaceImpl storage;