From 09cb75d4c6e020a9e54d6d4f5062ccb7bdcd999e Mon Sep 17 00:00:00 2001
From: Pierlauro Sciarelli <pierlauro.sciarelli@mongodb.com>
Date: Mon, 13 Feb 2023 14:13:05 +0000
Subject: SERVER-73385 Releasing unheld critical section upon sharded rename
 error must result in a no-op

---
 .../fsm_workloads/rename_sharded_collection.js     |  4 ---
 src/mongo/db/s/rename_collection_coordinator.cpp   | 38 +++++++++++-----------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/jstests/concurrency/fsm_workloads/rename_sharded_collection.js b/jstests/concurrency/fsm_workloads/rename_sharded_collection.js
index 7b919761b83..49152c3dcbb 100644
--- a/jstests/concurrency/fsm_workloads/rename_sharded_collection.js
+++ b/jstests/concurrency/fsm_workloads/rename_sharded_collection.js
@@ -16,10 +16,6 @@
  *   does_not_support_add_remove_shards,
  *   # This test just performs rename operations that can't be executed in transactions
  *   does_not_support_transactions,
- *   # Can be removed once PM-1965-Milestone-1 is completed.
- *
- *   # TODO SERVER-73385 reenable when fixed.
- *   assumes_balancer_off,
  *  ]
  */
 
diff --git a/src/mongo/db/s/rename_collection_coordinator.cpp b/src/mongo/db/s/rename_collection_coordinator.cpp
index 4f76b5d232f..867e2d3b47f 100644
--- a/src/mongo/db/s/rename_collection_coordinator.cpp
+++ b/src/mongo/db/s/rename_collection_coordinator.cpp
@@ -203,7 +203,6 @@ ExecutorFuture<void> RenameCollectionCoordinator::_runImpl(
                         checkCollectionUUIDMismatch(
                             opCtx, fromNss, *coll, _doc.getExpectedSourceUUID());
 
-
                         uassert(ErrorCodes::NamespaceNotFound,
                                 str::stream() << "Collection " << fromNss << " doesn't exist.",
                                 coll.getCollection());
@@ -236,10 +235,13 @@ ExecutorFuture<void> RenameCollectionCoordinator::_runImpl(
                     _doc.setTargetUUID(getCollectionUUID(
                         opCtx, toNss, optTargetCollType, /*throwNotFound*/ false));
 
-                    auto criticalSection = ShardingRecoveryService::get(opCtx);
                     if (!targetIsSharded) {
                         // (SERVER-67325) Acquire critical section on the target collection in order
-                        // to disallow concurrent `createCollection`
+                        // to disallow concurrent `createCollection`. In case the collection does
+                        // not exist, it will be later released by the rename participant. In case
+                        // the collection exists and is unsharded, the critical section can be
+                        // released right away as the participant will re-acquire it when needed.
+                        auto criticalSection = ShardingRecoveryService::get(opCtx);
                         criticalSection->acquireRecoverableCriticalSectionBlockWrites(
                             opCtx,
                             toNss,
@@ -255,23 +257,17 @@ ExecutorFuture<void> RenameCollectionCoordinator::_runImpl(
                         uassert(ErrorCodes::NamespaceExists,
                                 str::stream() << "a view already exists with that name: " << toNss,
                                 !CollectionCatalog::get(opCtx)->lookupView(opCtx, toNss));
-                    }
 
-                    const bool targetExists = [&]() {
-                        if (targetIsSharded) {
-                            return true;
+                        if (CollectionCatalog::get(opCtx)->lookupCollectionByNamespace(opCtx,
+                                                                                       toNss)) {
+                            // Release the critical section because the unsharded target collection
+                            // already exists, hence no risk of concurrent `createCollection`
+                            criticalSection->releaseRecoverableCriticalSection(
+                                opCtx,
+                                toNss,
+                                criticalSectionReason,
+                                WriteConcerns::kLocalWriteConcern);
                         }
-                        auto collectionCatalog = CollectionCatalog::get(opCtx);
-                        auto targetColl =
-                            collectionCatalog->lookupCollectionByNamespace(opCtx, toNss);
-                        return (bool)targetColl;  // true if exists and is unsharded
-                    }();
-
-                    if (targetExists) {
-                        // Release the critical section because the target collection
-                        // already exists, hence no risk of concurrent `createCollection`
-                        criticalSection->releaseRecoverableCriticalSection(
-                            opCtx, toNss, criticalSectionReason, WriteConcerns::kLocalWriteConcern);
                     }
 
                     sharding_ddl_util::checkRenamePreconditions(
@@ -295,7 +291,11 @@ ExecutorFuture<void> RenameCollectionCoordinator::_runImpl(
                 } catch (const DBException&) {
                     auto criticalSection = ShardingRecoveryService::get(opCtx);
                     criticalSection->releaseRecoverableCriticalSection(
-                        opCtx, toNss, criticalSectionReason, WriteConcerns::kLocalWriteConcern);
+                        opCtx,
+                        toNss,
+                        criticalSectionReason,
+                        WriteConcerns::kLocalWriteConcern,
+                        false /* throwIfReasonDiffers */);
                     _completeOnError = true;
                     throw;
                 }
-- 
cgit v1.2.1