From 88ef24561ef69ac7756b80256a86515180b830a3 Mon Sep 17 00:00:00 2001 From: Benety Goh Date: Fri, 25 Aug 2017 14:44:42 -0400 Subject: SERVER-30371 rename across database does not make target collection temporary if source collection was not temporary --- src/mongo/db/catalog/rename_collection.cpp | 15 ++++- src/mongo/db/catalog/rename_collection_test.cpp | 88 ++++++++++++++++--------- 2 files changed, 69 insertions(+), 34 deletions(-) diff --git a/src/mongo/db/catalog/rename_collection.cpp b/src/mongo/db/catalog/rename_collection.cpp index 9dfcaad04c8..788b1bc4303 100644 --- a/src/mongo/db/catalog/rename_collection.cpp +++ b/src/mongo/db/catalog/rename_collection.cpp @@ -246,8 +246,11 @@ Status renameCollectionCommon(OperationContext* opCtx, const auto& tmpName = tmpNameResult.getValue(); Collection* tmpColl = nullptr; OptionalCollectionUUID newUUID; + bool isSourceCollectionTemporary = false; { auto collectionOptions = sourceColl->getCatalogEntry()->getCollectionOptions(opCtx); + isSourceCollectionTemporary = collectionOptions.temp; + // Renaming across databases will result in a new UUID, as otherwise we'd require // two collections with the same uuid (temporarily). collectionOptions.temp = true; @@ -344,9 +347,15 @@ Status renameCollectionCommon(OperationContext* opCtx, dropTargetUUID = targetColl->uuid(); status = targetDB->dropCollection(opCtx, target.ns()); } - if (status.isOK()) - status = - targetDB->renameCollection(opCtx, tmpName.ns(), target.ns(), options.stayTemp); + if (status.isOK()) { + // When renaming the temporary collection in the target database, we have to take + // into account the CollectionOptions.temp value of the source collection and the + // 'stayTemp' option requested by the caller. + // If the source collection is not temporary, the resulting target collection must + // not be temporary. + auto stayTemp = isSourceCollectionTemporary && options.stayTemp; + status = targetDB->renameCollection(opCtx, tmpName.ns(), target.ns(), stayTemp); + } if (status.isOK()) status = sourceDB->dropCollection(opCtx, source.ns()); diff --git a/src/mongo/db/catalog/rename_collection_test.cpp b/src/mongo/db/catalog/rename_collection_test.cpp index 24be79072bc..02edeeade89 100644 --- a/src/mongo/db/catalog/rename_collection_test.cpp +++ b/src/mongo/db/catalog/rename_collection_test.cpp @@ -229,18 +229,6 @@ bool _isTempCollection(OperationContext* opCtx, const NamespaceString& nss) { return options.temp; } -/** - * Creates a temporary collection. - */ -void _createTempCollection(OperationContext* opCtx, const NamespaceString& nss) { - CollectionOptions options; - options.temp = true; - _createCollection(opCtx, nss, options); - ASSERT_TRUE(_isTempCollection(opCtx, nss)) << "Created collection " << nss - << " but collection is not temporary per options " - << options.toBSON(); -} - TEST_F(RenameCollectionTest, RenameCollectionReturnsNamespaceNotFoundIfDatabaseDoesNotExist) { ASSERT_FALSE(AutoGetDb(_opCtx.get(), _sourceNss.db(), MODE_X).getDb()); ASSERT_EQUALS(ErrorCodes::NamespaceNotFound, @@ -408,28 +396,66 @@ DEATH_TEST_F(RenameCollectionTest, ASSERT_OK(renameCollectionForApplyOps(_opCtx.get(), dbName, {}, cmd, renameOpTime)); } -TEST_F(RenameCollectionTest, RenameCollectionMakesTargetNonTemporaryIfStayTempIsFalse) { - _createTempCollection(_opCtx.get(), _sourceNss); +void _testRenameCollectionStayTemp(OperationContext* opCtx, + const NamespaceString& sourceNss, + const NamespaceString& targetNss, + bool stayTemp, + bool isSourceCollectionTemporary) { + CollectionOptions collectionOptions; + collectionOptions.temp = isSourceCollectionTemporary; + _createCollection(opCtx, sourceNss, collectionOptions); + RenameCollectionOptions options; - ASSERT_FALSE(options.stayTemp); - ASSERT_OK(renameCollection(_opCtx.get(), _sourceNss, _targetNss, options)); - ASSERT_FALSE(_collectionExists(_opCtx.get(), _sourceNss)) - << "source collection " << _sourceNss << " still exists after successful rename"; - ASSERT_FALSE(_isTempCollection(_opCtx.get(), _targetNss)) - << "target collection " << _targetNss - << " is still temporary after rename with stayTemp set to false."; + options.stayTemp = stayTemp; + ASSERT_OK(renameCollection(opCtx, sourceNss, targetNss, options)); + ASSERT_FALSE(_collectionExists(opCtx, sourceNss)) << "source collection " << sourceNss + << " still exists after successful rename"; + + if (!isSourceCollectionTemporary) { + ASSERT_FALSE(_isTempCollection(opCtx, targetNss)) + << "target collection " << targetNss + << " cannot not be temporary after rename if source collection is not temporary."; + } else if (stayTemp) { + ASSERT_TRUE(_isTempCollection(opCtx, targetNss)) + << "target collection " << targetNss + << " is no longer temporary after rename with stayTemp set to true."; + } else { + ASSERT_FALSE(_isTempCollection(opCtx, targetNss)) + << "target collection " << targetNss + << " still temporary after rename with stayTemp set to false."; + } } -TEST_F(RenameCollectionTest, RenameCollectionKeepsTargetAsTemporaryIfStayTempIsTrue) { - _createTempCollection(_opCtx.get(), _sourceNss); - RenameCollectionOptions options; - options.stayTemp = true; - ASSERT_OK(renameCollection(_opCtx.get(), _sourceNss, _targetNss, options)); - ASSERT_FALSE(_collectionExists(_opCtx.get(), _sourceNss)) - << "source collection " << _sourceNss << " still exists after successful rename"; - ASSERT_TRUE(_isTempCollection(_opCtx.get(), _targetNss)) - << "target collection " << _targetNss - << " is no longer temporary after rename with stayTemp set to true."; +TEST_F(RenameCollectionTest, RenameSameDatabaseStayTempFalse) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNss, false, true); +} + +TEST_F(RenameCollectionTest, RenameSameDatabaseStayTempTrue) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNss, true, true); +} + +TEST_F(RenameCollectionTest, RenameDifferentDatabaseStayTempFalse) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNssDifferentDb, false, true); +} + +TEST_F(RenameCollectionTest, RenameDifferentDatabaseStayTempTrue) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNssDifferentDb, true, true); +} + +TEST_F(RenameCollectionTest, RenameSameDatabaseStayTempFalseSourceNotTemporary) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNss, false, false); +} + +TEST_F(RenameCollectionTest, RenameSameDatabaseStayTempTrueSourceNotTemporary) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNss, true, false); +} + +TEST_F(RenameCollectionTest, RenameDifferentDatabaseStayTempFalseSourceNotTemporary) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNssDifferentDb, false, false); +} + +TEST_F(RenameCollectionTest, RenameDifferentDatabaseStayTempTrueSourceNotTemporary) { + _testRenameCollectionStayTemp(_opCtx.get(), _sourceNss, _targetNssDifferentDb, true, false); } } // namespace -- cgit v1.2.1