From 8281fff6b58911c84b0ba53a4a557ebf4dced10b Mon Sep 17 00:00:00 2001 From: Geert Bosch Date: Sat, 8 Jul 2017 00:24:03 -0400 Subject: SERVER-30106 Allow syncTail replication to write to drop-pending collections Reinstated after revert in a1c67941bf08c69cab04eba20bc9ce9a763e1c7f --- src/mongo/db/cloner.cpp | 2 +- src/mongo/db/concurrency/lock_state.cpp | 2 + src/mongo/db/namespace_string.cpp | 54 +++++++++++------------ src/mongo/db/namespace_string.h | 11 ++--- src/mongo/db/query/get_executor.cpp | 50 +++++++-------------- src/mongo/db/repl/databases_cloner.cpp | 2 +- src/mongo/db/repl/storage_interface_impl.cpp | 2 +- src/mongo/db/repl/storage_interface_impl_test.cpp | 4 +- 8 files changed, 56 insertions(+), 71 deletions(-) (limited to 'src/mongo') 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> 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::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 .system.users is ok for regular clients to update. -*/ -bool legalClientSystemNS(StringData ns); - /* e.g. NamespaceString ns("acme.orders"); cout << ns.coll; // "orders" @@ -226,6 +221,12 @@ public: bool isListCollectionsCursorNS() const; bool isListIndexesCursorNS() const; + /** + * Returns true if a client can modify this namespace even though it is under ".system." + * For example .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 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> 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> 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> 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> 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> 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( 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> 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> projStatus = applyProjection(opCtx, - nsString, - cq.get(), - request->getProj(), - allowPositional, - ws.get(), - std::move(root)); + StatusWith> 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; -- cgit v1.2.1