From d2f52a64e26caaedc2bfe988c9b7f22c2fb4e811 Mon Sep 17 00:00:00 2001 From: Maria van Keulen Date: Fri, 13 Mar 2020 16:34:22 -0400 Subject: SERVER-46966 Handle implicit collection creation errors during insert --- jstests/core/txns/create_collection.js | 19 ++++++++ jstests/libs/create_collection_txn_helpers.js | 21 +++++++++ src/mongo/db/catalog/database_impl.cpp | 9 ++++ src/mongo/db/ops/write_ops_exec.cpp | 64 ++++++++++++++++++--------- src/mongo/db/storage/write_unit_of_work.cpp | 3 ++ 5 files changed, 94 insertions(+), 22 deletions(-) diff --git a/jstests/core/txns/create_collection.js b/jstests/core/txns/create_collection.js index b2a1f99df2e..7490fdf1606 100644 --- a/jstests/core/txns/create_collection.js +++ b/jstests/core/txns/create_collection.js @@ -10,6 +10,7 @@ "use strict"; load("jstests/libs/create_collection_txn_helpers.js"); +load("jstests/libs/fixture_helpers.js"); // for isMongos function runCollectionCreateTest(explicitCreate, upsert) { const session = db.getMongo().startSession(); @@ -79,6 +80,24 @@ function runCollectionCreateTest(explicitCreate, upsert) { assert.eq(sessionColl.find({}).itcount(), 0); assert.eq(secondSessionColl.find({}).itcount(), 0); + sessionColl.drop({writeConcern: {w: "majority"}}); + + // mongos does not support throwWCEDuringTxnCollCreate + if (!FixtureHelpers.isMongos(db)) { + jsTest.log( + "Testing createCollection with writeConflict errors in a transaction (SHOULD ABORT"); + assert.commandWorked( + db.adminCommand({configureFailPoint: "throwWCEDuringTxnCollCreate", mode: "alwaysOn"})); + session.startTransaction({writeConcern: {w: "majority"}}); + assertCollCreateFailedWithCode( + sessionDB, collName, explicitCreate, upsert, ErrorCodes.WriteConflict); + assert.commandFailedWithCode(session.abortTransaction_forTesting(), + ErrorCodes.NoSuchTransaction); + assert.eq(sessionColl.find({}).itcount(), 0); + assert.commandWorked( + db.adminCommand({configureFailPoint: "throwWCEDuringTxnCollCreate", mode: "off"})); + } + session.endSession(); } diff --git a/jstests/libs/create_collection_txn_helpers.js b/jstests/libs/create_collection_txn_helpers.js index 08d7093a5c7..3b113ebd5f8 100644 --- a/jstests/libs/create_collection_txn_helpers.js +++ b/jstests/libs/create_collection_txn_helpers.js @@ -25,3 +25,24 @@ const createCollAndCRUDInTxn = function(sessionDB, collName, explicitCreate, ups assert.commandWorked(sessionColl.deleteOne({_id: 2})); assert.eq(sessionColl.find({}).itcount(), 1); }; + +const assertCollCreateFailedWithCode = function(sessionDB, collName, explicitCreate, upsert, code) { + if (undefined === explicitCreate) { + doassert('assertWriteConflictForCollCreate called with undefined explicitCreate'); + } + if (undefined === upsert) { + doassert('assertWriteConflictForCollCreate called with undefined upsert'); + } + if (undefined === code) { + doassert('assertWriteConflictForCollCreate called with undefined code'); + } + let sessionColl = sessionDB[collName]; + if (explicitCreate) { + assert.commandFailedWithCode(sessionDB.createCollection(collName), code); + } else if (upsert) { + assert.commandFailedWithCode(sessionColl.update({_id: 1}, {$inc: {a: 1}}, {upsert: true}), + code); + } else { + assert.commandFailedWithCode(sessionColl.insert({a: 1}), code); + } +}; diff --git a/src/mongo/db/catalog/database_impl.cpp b/src/mongo/db/catalog/database_impl.cpp index 5ea7746c8ce..3339be94676 100644 --- a/src/mongo/db/catalog/database_impl.cpp +++ b/src/mongo/db/catalog/database_impl.cpp @@ -82,6 +82,7 @@ namespace mongo { namespace { +MONGO_FAIL_POINT_DEFINE(throwWCEDuringTxnCollCreate); MONGO_FAIL_POINT_DEFINE(hangBeforeLoggingCreateCollection); MONGO_FAIL_POINT_DEFINE(hangAndFailAfterCreateCollectionReservesOpTime); MONGO_FAIL_POINT_DEFINE(openCreateCollectionWindowFp); @@ -573,6 +574,14 @@ void DatabaseImpl::_checkCanCreateCollection(OperationContext* opCtx, } } + if (MONGO_unlikely(throwWCEDuringTxnCollCreate.shouldFail()) && + opCtx->inMultiDocumentTransaction()) { + LOGV2(4696600, + "Throwing WriteConflictException due to failpoint 'throwWCEDuringTxnCollCreate'"); + throw WriteConflictException(); + } + + uassert(17320, str::stream() << "cannot do createCollection on namespace with a $ in it: " << nss, nss.ns().find('$') == std::string::npos); diff --git a/src/mongo/db/ops/write_ops_exec.cpp b/src/mongo/db/ops/write_ops_exec.cpp index cf08d7497d5..cd499617d1f 100644 --- a/src/mongo/db/ops/write_ops_exec.cpp +++ b/src/mongo/db/ops/write_ops_exec.cpp @@ -432,31 +432,51 @@ bool insertBatchAndHandleErrors(OperationContext* opCtx, &hangWithLockDuringBatchInsert, opCtx, "hangWithLockDuringBatchInsert"); }; + auto txnParticipant = TransactionParticipant::get(opCtx); + auto inTxn = txnParticipant && opCtx->inMultiDocumentTransaction(); + bool shouldProceedWithBatchInsert = true; + try { acquireCollection(); - auto txnParticipant = TransactionParticipant::get(opCtx); - auto inTxn = txnParticipant && opCtx->inMultiDocumentTransaction(); - if (!collection->getCollection()->isCapped() && !inTxn && batch.size() > 1) { - // First try doing it all together. If all goes well, this is all we need to do. - // See Collection::_insertDocuments for why we do all capped inserts one-at-a-time. - lastOpFixer->startingOp(); - insertDocuments( - opCtx, collection->getCollection(), batch.begin(), batch.end(), fromMigrate); - lastOpFixer->finishedOpSuccessfully(); - globalOpCounters.gotInserts(batch.size()); - ServerWriteConcernMetrics::get(opCtx)->recordWriteConcernForInserts( - opCtx->getWriteConcern(), batch.size()); - SingleWriteResult result; - result.setN(1); - - std::fill_n(std::back_inserter(out->results), batch.size(), std::move(result)); - curOp.debug().additiveMetrics.incrementNinserted(batch.size()); - return true; - } - } catch (const DBException&) { - // Ignore this failure and behave as if we never tried to do the combined batch - // insert. The loop below will handle reporting any non-transient errors. + } catch (const DBException& ex) { collection.reset(); + if (inTxn) { + // It is not safe to ignore errors from collection creation while inside a + // multi-document transaction. + auto canContinue = + handleError(opCtx, ex, wholeOp.getNamespace(), wholeOp.getWriteCommandBase(), out); + invariant(!canContinue); + return false; + } + // Otherwise, proceed as though the batch insert block failed, since the batch insert block + // assumes `acquireCollection` is successful. + shouldProceedWithBatchInsert = false; + } + + if (shouldProceedWithBatchInsert) { + try { + if (!collection->getCollection()->isCapped() && !inTxn && batch.size() > 1) { + // First try doing it all together. If all goes well, this is all we need to do. + // See Collection::_insertDocuments for why we do all capped inserts one-at-a-time. + lastOpFixer->startingOp(); + insertDocuments( + opCtx, collection->getCollection(), batch.begin(), batch.end(), fromMigrate); + lastOpFixer->finishedOpSuccessfully(); + globalOpCounters.gotInserts(batch.size()); + ServerWriteConcernMetrics::get(opCtx)->recordWriteConcernForInserts( + opCtx->getWriteConcern(), batch.size()); + SingleWriteResult result; + result.setN(1); + + std::fill_n(std::back_inserter(out->results), batch.size(), std::move(result)); + curOp.debug().additiveMetrics.incrementNinserted(batch.size()); + return true; + } + } catch (const DBException&) { + // Ignore this failure and behave as if we never tried to do the combined batch + // insert. The loop below will handle reporting any non-transient errors. + collection.reset(); + } } // Try to insert the batch one-at-a-time. This path is executed for singular batches, diff --git a/src/mongo/db/storage/write_unit_of_work.cpp b/src/mongo/db/storage/write_unit_of_work.cpp index aaa7c9fcfb3..bdbda47471a 100644 --- a/src/mongo/db/storage/write_unit_of_work.cpp +++ b/src/mongo/db/storage/write_unit_of_work.cpp @@ -53,6 +53,9 @@ WriteUnitOfWork::WriteUnitOfWork(OperationContext* opCtx) _opCtx->recoveryUnit()->beginUnitOfWork(_opCtx); _opCtx->_ruState = RecoveryUnitState::kActiveUnitOfWork; } + // Make sure we don't silently proceed after a previous WriteUnitOfWork under the same parent + // WriteUnitOfWork fails. + invariant(_opCtx->_ruState != RecoveryUnitState::kFailedUnitOfWork); } WriteUnitOfWork::~WriteUnitOfWork() { -- cgit v1.2.1