diff options
-rw-r--r-- | src/mongo/base/error_codes.err | 1 | ||||
-rw-r--r-- | src/mongo/db/catalog/SConscript | 2 | ||||
-rw-r--r-- | src/mongo/db/catalog/multi_index_block.h | 28 | ||||
-rw-r--r-- | src/mongo/db/catalog/multi_index_block_impl.cpp | 109 | ||||
-rw-r--r-- | src/mongo/db/catalog/multi_index_block_impl.h | 52 | ||||
-rw-r--r-- | src/mongo/db/catalog/multi_index_block_test.cpp | 204 |
6 files changed, 384 insertions, 12 deletions
diff --git a/src/mongo/base/error_codes.err b/src/mongo/base/error_codes.err index 5dcfaf5d878..ca74eb7a13b 100644 --- a/src/mongo/base/error_codes.err +++ b/src/mongo/base/error_codes.err @@ -273,6 +273,7 @@ error_code("MigrationConflict", 272) error_code("ProducerConsumerQueueProducerQueueDepthExceeded", 273) error_code("ProducerConsumerQueueConsumed", 274) error_code("ExchangePassthrough", 275) # For exchange execution in aggregation. Do not reuse. +error_code("IndexBuildAborted", 276) # Error codes 4000-8999 are reserved. # Non-sequential error codes (for compatibility only) diff --git a/src/mongo/db/catalog/SConscript b/src/mongo/db/catalog/SConscript index 7195387ad6c..06ef36e5bce 100644 --- a/src/mongo/db/catalog/SConscript +++ b/src/mongo/db/catalog/SConscript @@ -235,6 +235,8 @@ env.CppUnitTest( 'collection', 'multi_index_block', '$BUILD_DIR/mongo/db/namespace_string', + '$BUILD_DIR/mongo/db/repl/replmocks', + '$BUILD_DIR/mongo/db/service_context_test_fixture', ], ) diff --git a/src/mongo/db/catalog/multi_index_block.h b/src/mongo/db/catalog/multi_index_block.h index 4e72de1024d..70d2d4f0ce9 100644 --- a/src/mongo/db/catalog/multi_index_block.h +++ b/src/mongo/db/catalog/multi_index_block.h @@ -37,6 +37,7 @@ #include "mongo/base/disallow_copying.h" #include "mongo/base/status.h" +#include "mongo/base/string_data.h" #include "mongo/db/catalog/index_catalog.h" #include "mongo/db/index/index_access_method.h" #include "mongo/db/record_id.h" @@ -169,6 +170,31 @@ public: virtual Status commit(stdx::function<void(const BSONObj& spec)> onCreateFn) = 0; /** + * Returns true if this index builder was added to the index catalog successfully. + * In addition to having commit() return without errors, the enclosing WUOW has to be committed + * for the indexes to show up in the index catalog. + */ + virtual bool isCommitted() const = 0; + + /** + * Signals the index build to abort. + * + * In-progress inserts and commits will still run to completion. However, subsequent index build + * operations will fail an IndexBuildAborted error. + * + * Aborts the uncommitted index build and prevents further inserts or commit attempts from + * proceeding. On destruction, all traces of uncommitted index builds will be removed. + * + * If the index build has already been aborted (using abort() or abortWithoutCleanup()), + * this function does nothing. + * + * If this index build has been committed successfully, this function has no effect. + * + * May be called from any thread. + */ + virtual void abort(StringData reason) = 0; + + /** * May be called at any time after construction but before a successful commit(). Suppresses * the default behavior on destruction of removing all traces of uncommitted index builds. * @@ -181,6 +207,8 @@ public: * * Does not matter whether it is called inside of a WriteUnitOfWork. Will not be rolled * back. + * + * Must be called from owning thread. */ virtual void abortWithoutCleanup() = 0; diff --git a/src/mongo/db/catalog/multi_index_block_impl.cpp b/src/mongo/db/catalog/multi_index_block_impl.cpp index 941b89e6c17..28f90ae1249 100644 --- a/src/mongo/db/catalog/multi_index_block_impl.cpp +++ b/src/mongo/db/catalog/multi_index_block_impl.cpp @@ -34,6 +34,8 @@ #include "mongo/db/catalog/multi_index_block_impl.h" +#include <ostream> + #include "mongo/base/error_codes.h" #include "mongo/base/init.h" #include "mongo/db/audit.h" @@ -50,6 +52,7 @@ #include "mongo/db/operation_context.h" #include "mongo/db/repl/replication_coordinator.h" #include "mongo/db/server_parameters.h" +#include "mongo/logger/redaction.h" #include "mongo/stdx/memory.h" #include "mongo/stdx/mutex.h" #include "mongo/util/fail_point.h" @@ -209,6 +212,19 @@ StatusWith<std::vector<BSONObj>> MultiIndexBlockImpl::init(const BSONObj& spec) } StatusWith<std::vector<BSONObj>> MultiIndexBlockImpl::init(const std::vector<BSONObj>& indexSpecs) { + if (State::kAborted == _getState()) { + return {ErrorCodes::IndexBuildAborted, + str::stream() << "Index build aborted: " << _abortReason + << ". Cannot initialize index builder: " + << _collection->ns().ns() + << "(" + << *_collection->uuid() + << "): " + << indexSpecs.size() + << " provided. First index spec: " + << (indexSpecs.empty() ? BSONObj() : indexSpecs[0])}; + } + WriteUnitOfWork wunit(_opCtx); invariant(_indexes.empty()); @@ -313,6 +329,8 @@ StatusWith<std::vector<BSONObj>> MultiIndexBlockImpl::init(const std::vector<BSO } } + _setState(State::kRunning); + return indexInfoObjs; } @@ -481,6 +499,17 @@ Status MultiIndexBlockImpl::insertAllDocumentsInCollection() { Status MultiIndexBlockImpl::insert(const BSONObj& doc, const RecordId& loc, std::vector<BSONObj>* const dupKeysInserted) { + if (State::kAborted == _getState()) { + return {ErrorCodes::IndexBuildAborted, + str::stream() << "Index build aborted: " << _abortReason + << ". Cannot insert document into index builder: " + << _collection->ns().ns() + << "(" + << *_collection->uuid() + << "): " + << redact(doc)}; + } + for (size_t i = 0; i < _indexes.size(); i++) { if (_indexes[i].filterExpression && !_indexes[i].filterExpression->matchesBSON(doc)) { continue; @@ -516,9 +545,18 @@ Status MultiIndexBlockImpl::doneInserting(std::set<RecordId>* dupRecords) { Status MultiIndexBlockImpl::doneInserting(std::vector<BSONObj>* dupKeysInserted) { return _doneInserting(nullptr, dupKeysInserted); } - Status MultiIndexBlockImpl::_doneInserting(std::set<RecordId>* dupRecords, std::vector<BSONObj>* dupKeysInserted) { + if (State::kAborted == _getState()) { + return {ErrorCodes::IndexBuildAborted, + str::stream() << "Index build aborted: " << _abortReason + << ". Cannot complete insertion phase: " + << _collection->ns().ns() + << "(" + << *_collection->uuid() + << ")"}; + } + invariant(_opCtx->lockState()->isNoop() || !_opCtx->lockState()->inAWriteUnitOfWork()); for (size_t i = 0; i < _indexes.size(); i++) { if (_indexes[i].bulk == NULL) @@ -536,10 +574,13 @@ Status MultiIndexBlockImpl::_doneInserting(std::set<RecordId>* dupRecords, } } + _setState(State::kPreCommit); + return Status::OK(); } void MultiIndexBlockImpl::abortWithoutCleanup() { + _setStateToAbortedIfNotCommitted("aborted without cleanup"_sd); _indexes.clear(); _needToCleanup = false; } @@ -549,6 +590,16 @@ Status MultiIndexBlockImpl::commit() { } Status MultiIndexBlockImpl::commit(stdx::function<void(const BSONObj& spec)> onCreateFn) { + if (State::kAborted == _getState()) { + return {ErrorCodes::IndexBuildAborted, + str::stream() << "Index build aborted: " << _abortReason + << ". Cannot commit index builder: " + << _collection->ns().ns() + << "(" + << *_collection->uuid() + << ")"}; + } + // Do not interfere with writing multikey information when committing index builds. auto restartTracker = MakeGuard([this] { MultikeyPathTracker::get(_opCtx).startTrackingMultikeyPathInfo(); }); @@ -582,10 +633,66 @@ Status MultiIndexBlockImpl::commit(stdx::function<void(const BSONObj& spec)> onC } } + // The state of this index build is set to Committed only when the WUOW commits. + // It is possible for abort() to be called after the check at the beginning of this function and + // before the WUOW is committed. If the WUOW commits, the final state of this index builder will + // be Committed. Otherwise, the index builder state will remain as Aborted and further attempts + // to commit this index build will fail. _opCtx->recoveryUnit()->registerChange(new SetNeedToCleanupOnRollback(this)); + _opCtx->recoveryUnit()->onCommit( + [this](boost::optional<Timestamp> commitTime) { this->_setState(State::kCommitted); }); _needToCleanup = false; return Status::OK(); } +bool MultiIndexBlockImpl::isCommitted() const { + return State::kCommitted == _getState(); +} + +void MultiIndexBlockImpl::abort(StringData reason) { + _setStateToAbortedIfNotCommitted(reason); +} + + +MultiIndexBlockImpl::State MultiIndexBlockImpl::getState_forTest() const { + return _getState(); +} + +MultiIndexBlockImpl::State MultiIndexBlockImpl::_getState() const { + stdx::lock_guard<stdx::mutex> lock(_mutex); + return _state; +} + +void MultiIndexBlockImpl::_setState(State newState) { + invariant(State::kAborted != newState); + stdx::lock_guard<stdx::mutex> lock(_mutex); + _state = newState; +} + +void MultiIndexBlockImpl::_setStateToAbortedIfNotCommitted(StringData reason) { + stdx::lock_guard<stdx::mutex> lock(_mutex); + if (State::kCommitted == _state) { + return; + } + _state = State::kAborted; + _abortReason = reason.toString(); +} + +std::ostream& operator<<(std::ostream& os, const MultiIndexBlockImpl::State& state) { + switch (state) { + case MultiIndexBlockImpl::State::kUninitialized: + return os << "Uninitialized"; + case MultiIndexBlockImpl::State::kRunning: + return os << "Running"; + case MultiIndexBlockImpl::State::kPreCommit: + return os << "PreCommit"; + case MultiIndexBlockImpl::State::kCommitted: + return os << "Committed"; + case MultiIndexBlockImpl::State::kAborted: + return os << "Aborted"; + } + MONGO_UNREACHABLE; +} + } // namespace mongo diff --git a/src/mongo/db/catalog/multi_index_block_impl.h b/src/mongo/db/catalog/multi_index_block_impl.h index b2a5c09110e..a1c7f726cba 100644 --- a/src/mongo/db/catalog/multi_index_block_impl.h +++ b/src/mongo/db/catalog/multi_index_block_impl.h @@ -31,6 +31,7 @@ #include "mongo/db/catalog/multi_index_block.h" +#include <iosfwd> #include <memory> #include <set> #include <string> @@ -42,6 +43,7 @@ #include "mongo/db/catalog/index_catalog_impl.h" #include "mongo/db/index/index_access_method.h" #include "mongo/db/record_id.h" +#include "mongo/stdx/mutex.h" namespace mongo { @@ -90,12 +92,37 @@ public: Status commit() override; Status commit(stdx::function<void(const BSONObj& spec)> onCreateFn) override; + bool isCommitted() const override; + + void abort(StringData reason) override; + void abortWithoutCleanup() override; bool getBuildInBackground() const override { return _buildInBackground; } + /** + * State transitions: + * + * Uninitialized --> Running --> PreCommit --> Committed + * | | | ^ + * | | | | + * \--------------+------------+-------> Aborted + * + * It is possible for abort() to skip intermediate states. For example, calling abort() when the + * index build has not been initialized will transition from Uninitialized directly to Aborted. + * + * In the case where we are in the midst of committing the WUOW for a successful commit() call, + * we may transition temporarily to Aborted before finally ending at Committed. See comments for + * MultiIndexBlock::abort(). + * + * Not part of MultiIndexBlock interface. Callers should not have to query the state of the + * MultiIndexBlock directly. + */ + enum class State { kUninitialized, kRunning, kPreCommit, kCommitted, kAborted }; + State getState_forTest() const; + private: class SetNeedToCleanupOnRollback; class CleanupIndexesVectorOnRollback; @@ -112,6 +139,21 @@ private: Status _doneInserting(std::set<RecordId>* dupRecords, std::vector<BSONObj>* dupKeysInserted); + /** + * Returns the current state. + */ + State _getState() const; + + /** + * Updates the current state to a non-Aborted state. + */ + void _setState(State newState); + + /** + * Updates the current state to Aborted with the given reason. + */ + void _setStateToAbortedIfNotCommitted(StringData reason); + std::vector<IndexToBuild> _indexes; std::unique_ptr<BackgroundOperation> _backgroundOperation; @@ -125,6 +167,16 @@ private: bool _ignoreUnique; bool _needToCleanup; + + // Protects member variables of this class declared below. + mutable stdx::mutex _mutex; + + State _state = State::kUninitialized; + std::string _abortReason; }; +// For unit tests that need to check MultiIndexBlock states. +// The ASSERT_*() macros use this function to print the value of 'state' when the predicate fails. +std::ostream& operator<<(std::ostream& os, const MultiIndexBlockImpl::State& state); + } // namespace mongo diff --git a/src/mongo/db/catalog/multi_index_block_test.cpp b/src/mongo/db/catalog/multi_index_block_test.cpp index 4f09562a847..11dcd74502c 100644 --- a/src/mongo/db/catalog/multi_index_block_test.cpp +++ b/src/mongo/db/catalog/multi_index_block_test.cpp @@ -28,29 +28,211 @@ * it in the license file. */ -#include "mongo/db/catalog/multi_index_block.h" - -#include "mongo/db/catalog/collection.h" #include "mongo/db/catalog/collection_mock.h" +#include "mongo/db/catalog/index_catalog_noop.h" #include "mongo/db/catalog/multi_index_block_impl.h" -#include "mongo/db/operation_context_noop.h" +#include "mongo/db/jsobj.h" +#include "mongo/db/repl/replication_coordinator_mock.h" +#include "mongo/db/service_context_test_fixture.h" #include "mongo/unittest/unittest.h" namespace mongo { namespace { /** - * Simple initialization test for MultiIndexBlock to check library dependencies. - * For greater test coverage, it may be necessary to make this test fixture inherit from + * Unit test for MultiIndexBlock to verify basic functionality. + * Using a mocked Collection object ensures that we are pulling in a minimal set of library + * dependencies. + * For integration tests, it may be necessary to make this test fixture inherit from * ServiceContextMongoDTest. */ -class MultiIndexBlockTest : public unittest::Test {}; +class MultiIndexBlockTest : public ServiceContextTest { +private: + void setUp() override; + void tearDown() override; + +protected: + OperationContext* getOpCtx() const; + Collection* getCollection() const; + MultiIndexBlockImpl* getIndexer() const; + +private: + ServiceContext::UniqueOperationContext _opCtx; + std::unique_ptr<Collection> _collection; + std::unique_ptr<MultiIndexBlockImpl> _indexer; +}; + +void MultiIndexBlockTest::setUp() { + ServiceContextTest::setUp(); + + auto service = getServiceContext(); + repl::ReplicationCoordinator::set(service, + std::make_unique<repl::ReplicationCoordinatorMock>(service)); + + _opCtx = makeOperationContext(); -TEST_F(MultiIndexBlockTest, Create) { - OperationContextNoop opCtx; NamespaceString nss("mydb.mycoll"); - Collection collection(std::make_unique<CollectionMock>(nss)); - MultiIndexBlockImpl(&opCtx, &collection); + auto collectionMock = + std::make_unique<CollectionMock>(nss, std::make_unique<IndexCatalogNoop>()); + _collection = std::make_unique<Collection>(std::move(collectionMock)); + + _indexer = std::make_unique<MultiIndexBlockImpl>(_opCtx.get(), _collection.get()); +} + +void MultiIndexBlockTest::tearDown() { + auto service = getServiceContext(); + repl::ReplicationCoordinator::set(service, {}); + + _indexer = {}; + + _collection = {}; + + _opCtx = {}; + + ServiceContextTest::tearDown(); +} + +OperationContext* MultiIndexBlockTest::getOpCtx() const { + return _opCtx.get(); +} + +Collection* MultiIndexBlockTest::getCollection() const { + return _collection.get(); +} + +MultiIndexBlockImpl* MultiIndexBlockTest::getIndexer() const { + return _indexer.get(); +} + +TEST_F(MultiIndexBlockTest, CommitWithoutInsertingDocuments) { + auto indexer = getIndexer(); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kUninitialized, indexer->getState_forTest()); + + auto specs = unittest::assertGet(indexer->init(std::vector<BSONObj>())); + ASSERT_EQUALS(0U, specs.size()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kRunning, indexer->getState_forTest()); + + ASSERT_OK(indexer->doneInserting()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kPreCommit, indexer->getState_forTest()); + + ASSERT_FALSE(indexer->isCommitted()); + { + WriteUnitOfWork wunit(getOpCtx()); + ASSERT_OK(indexer->commit()); + wunit.commit(); + } + ASSERT(indexer->isCommitted()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kCommitted, indexer->getState_forTest()); +} + +TEST_F(MultiIndexBlockTest, CommitAfterInsertingSingleDocument) { + auto indexer = getIndexer(); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kUninitialized, indexer->getState_forTest()); + + auto specs = unittest::assertGet(indexer->init(std::vector<BSONObj>())); + ASSERT_EQUALS(0U, specs.size()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kRunning, indexer->getState_forTest()); + + ASSERT_OK(indexer->insert({}, {}, nullptr)); + ASSERT_OK(indexer->doneInserting()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kPreCommit, indexer->getState_forTest()); + + ASSERT_FALSE(indexer->isCommitted()); + { + WriteUnitOfWork wunit(getOpCtx()); + ASSERT_OK(indexer->commit()); + wunit.commit(); + } + ASSERT(indexer->isCommitted()); + + // abort() should have no effect after the index build is committed. + indexer->abort("test"_sd); + ASSERT(indexer->isCommitted()); +} + +TEST_F(MultiIndexBlockTest, AbortWithoutCleanupAfterInsertingSingleDocument) { + auto indexer = getIndexer(); + auto specs = unittest::assertGet(indexer->init(std::vector<BSONObj>())); + ASSERT_EQUALS(0U, specs.size()); + ASSERT_OK(indexer->insert({}, {}, nullptr)); + indexer->abortWithoutCleanup(); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_FALSE(indexer->isCommitted()); +} + +TEST_F(MultiIndexBlockTest, InitFailsAfterAbort) { + auto indexer = getIndexer(); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kUninitialized, indexer->getState_forTest()); + + indexer->abort("test"_sd); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_EQUALS(ErrorCodes::IndexBuildAborted, indexer->init(std::vector<BSONObj>()).getStatus()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_FALSE(indexer->isCommitted()); +} + +TEST_F(MultiIndexBlockTest, InsertingSingleDocumentFailsAfterAbort) { + auto indexer = getIndexer(); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kUninitialized, indexer->getState_forTest()); + + auto specs = unittest::assertGet(indexer->init(std::vector<BSONObj>())); + ASSERT_EQUALS(0U, specs.size()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kRunning, indexer->getState_forTest()); + + indexer->abort("test"_sd); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_EQUALS(ErrorCodes::IndexBuildAborted, + indexer->insert(BSON("_id" << 123 << "a" << 456), {}, nullptr)); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_FALSE(indexer->isCommitted()); +} + +TEST_F(MultiIndexBlockTest, DoneInsertingFailsAfterAbort) { + auto indexer = getIndexer(); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kUninitialized, indexer->getState_forTest()); + + auto specs = unittest::assertGet(indexer->init(std::vector<BSONObj>())); + ASSERT_EQUALS(0U, specs.size()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kRunning, indexer->getState_forTest()); + + ASSERT_OK(indexer->insert(BSON("_id" << 123 << "a" << 456), {}, nullptr)); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kRunning, indexer->getState_forTest()); + + indexer->abort("test"_sd); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_EQUALS(ErrorCodes::IndexBuildAborted, indexer->doneInserting()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_FALSE(indexer->isCommitted()); +} + +TEST_F(MultiIndexBlockTest, CommitFailsAfterAbort) { + auto indexer = getIndexer(); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kUninitialized, indexer->getState_forTest()); + + auto specs = unittest::assertGet(indexer->init(std::vector<BSONObj>())); + ASSERT_EQUALS(0U, specs.size()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kRunning, indexer->getState_forTest()); + + ASSERT_OK(indexer->insert(BSON("_id" << 123 << "a" << 456), {}, nullptr)); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kRunning, indexer->getState_forTest()); + + ASSERT_OK(indexer->doneInserting()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kPreCommit, indexer->getState_forTest()); + + indexer->abort("test"_sd); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_EQUALS(ErrorCodes::IndexBuildAborted, indexer->commit()); + ASSERT_EQUALS(MultiIndexBlockImpl::State::kAborted, indexer->getState_forTest()); + + ASSERT_FALSE(indexer->isCommitted()); } } // namespace |