From 9c49d721526ac83ada34950841ceef5b0b48c3c5 Mon Sep 17 00:00:00 2001 From: Louis Williams Date: Tue, 15 Jan 2019 11:25:41 -0500 Subject: SERVER-38748 Background indexes created through applyOps should run on the command thread --- jstests/core/apply_ops_invalid_index_spec.js | 18 ++++++ jstests/noPassthrough/indexbg_killop_apply_ops.js | 29 +++++---- src/mongo/db/repl/oplog.cpp | 73 +++++++++++++---------- src/mongo/dbtests/storage_timestamp_tests.cpp | 23 ++----- 4 files changed, 84 insertions(+), 59 deletions(-) diff --git a/jstests/core/apply_ops_invalid_index_spec.js b/jstests/core/apply_ops_invalid_index_spec.js index 01a4fab3a63..5ed9e6d8ee6 100644 --- a/jstests/core/apply_ops_invalid_index_spec.js +++ b/jstests/core/apply_ops_invalid_index_spec.js @@ -50,6 +50,24 @@ }), ErrorCodes.InvalidIndexSpecificationOption); + // A createIndexes command for a background index with unknown field in the index spec should + // fail. + assert.commandFailedWithCode(db.adminCommand({ + applyOps: [{ + op: 'c', + ns: cmdNs, + o: { + createIndexes: t.getName(), + v: 2, + key: {a: 1}, + background: true, + name: 'a_1_background', + unknown: 1, + }, + }], + }), + ErrorCodes.InvalidIndexSpecificationOption); + // A createIndexes command for a v:1 index with an unknown field in the index spec should work. const res1 = assert.commandWorked(db.adminCommand({ applyOps: [{ diff --git a/jstests/noPassthrough/indexbg_killop_apply_ops.js b/jstests/noPassthrough/indexbg_killop_apply_ops.js index d99e45f65f6..d7bb3da18a9 100644 --- a/jstests/noPassthrough/indexbg_killop_apply_ops.js +++ b/jstests/noPassthrough/indexbg_killop_apply_ops.js @@ -1,6 +1,8 @@ /** - * Confirms that background index builds started through applyOps on a primary cannot be aborted - * using killop. + * Confirms that background index builds started through applyOps on a primary can be aborted using + * killop. This is because primaries run background index builds in the foreground when run through + * applyOps. + * * @tags: [requires_replication] */ (function() { @@ -46,25 +48,32 @@ }, ] }; - const createIdx = - startParallelShell('db.adminCommand(' + tojson(applyOpsCmd) + ')', primary.port); + const createIdx = startParallelShell( + 'assert.commandWorked(db.adminCommand(' + tojson(applyOpsCmd) + '))', primary.port); // When the index build starts, find its op id. const opId = IndexBuildTest.waitForIndexBuildToStart(testDB); + // CurOp output should contain the current state of the index builder such as the build UUID + // and build phase. + IndexBuildTest.assertIndexBuildCurrentOpContents(testDB, opId, false); + // Kill the index build. This should have no effect. assert.commandWorked(testDB.killOp(opId)); // Wait for the index build to stop. - IndexBuildTest.resumeIndexBuilds(primary); - IndexBuildTest.waitForIndexBuildToStop(testDB); + try { + IndexBuildTest.waitForIndexBuildToStop(testDB); + } finally { + IndexBuildTest.resumeIndexBuilds(primary); + } - // Expect successful createIndex command invocation in parallel shell because applyOps returns - // immediately after starting the background index build in a separate thread. - createIdx(); + const exitCode = createIdx({checkExitSuccess: false}); + assert.neq( + 0, exitCode, 'expected shell to exit abnormally due to index build being terminated'); // Check that index was created on the primary despite the attempted killOp(). - IndexBuildTest.assertIndexes(coll, 2, ['_id_', 'a_1']); + IndexBuildTest.assertIndexes(coll, 1, ['_id_']); rst.stopSet(); })(); diff --git a/src/mongo/db/repl/oplog.cpp b/src/mongo/db/repl/oplog.cpp index 428bee03dfd..27a1f77282c 100644 --- a/src/mongo/db/repl/oplog.cpp +++ b/src/mongo/db/repl/oplog.cpp @@ -212,6 +212,33 @@ private: BSONObj _oField; }; +bool shouldBuildInForeground(OperationContext* opCtx, + const BSONObj& index, + const NamespaceString& indexNss, + repl::OplogApplication::Mode mode) { + if (mode == OplogApplication::Mode::kRecovering) { + LOG(3) << "apply op: building background index " << index + << " in the foreground because the node is in recovery"; + return true; + } + + if (!index["background"].trueValue()) { + return true; + } + + // Primaries should build indexes in the foreground because failures cannot be handled + // by the background thread. + const bool isPrimary = + repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, indexNss); + if (isPrimary) { + LOG(3) << "apply op: not building background index " << index + << " in a background thread because this is a primary"; + return true; + } + return false; +} + + } // namespace void setOplogCollectionName(ServiceContext* service) { @@ -255,41 +282,25 @@ void createIndexForApplyOps(OperationContext* opCtx, ? IndexBuilder::ReplicatedWrites::kReplicated : IndexBuilder::ReplicatedWrites::kUnreplicated; - if (indexSpec["background"].trueValue()) { - if (mode == OplogApplication::Mode::kRecovering) { - LOG(3) << "apply op: building background index " << indexSpec - << " in the foreground because the node is in recovery"; - IndexBuilder builder(indexSpec, constraints, replicatedWrites); - Status status = builder.buildInForeground(opCtx, db); - uassertStatusOK(status); - } else { - Lock::TempRelease release(opCtx->lockState()); - if (opCtx->lockState()->isLocked()) { - // If TempRelease fails, background index build will deadlock. - LOG(3) << "apply op: building background index " << indexSpec - << " in the foreground because temp release failed"; - IndexBuilder builder(indexSpec, constraints, replicatedWrites); - Status status = builder.buildInForeground(opCtx, db); - uassertStatusOK(status); - } else { - IndexBuilder* builder = - new IndexBuilder(indexSpec, - constraints, - replicatedWrites, - opCtx->recoveryUnit()->getCommitTimestamp()); - // This spawns a new thread and returns immediately. - builder->go(); - // Wait for thread to start and register itself - IndexBuilder::waitForBgIndexStarting(); - } - } - - opCtx->recoveryUnit()->abandonSnapshot(); - } else { + if (shouldBuildInForeground(opCtx, indexSpec, indexNss, mode)) { IndexBuilder builder(indexSpec, constraints, replicatedWrites); Status status = builder.buildInForeground(opCtx, db); uassertStatusOK(status); + } else { + Lock::TempRelease release(opCtx->lockState()); + // TempRelease cannot fail because no recursive locks should be taken. + invariant(!opCtx->lockState()->isLocked()); + + IndexBuilder* builder = new IndexBuilder( + indexSpec, constraints, replicatedWrites, opCtx->recoveryUnit()->getCommitTimestamp()); + // This spawns a new thread and returns immediately. + builder->go(); + // Wait for thread to start and register itself + IndexBuilder::waitForBgIndexStarting(); } + + opCtx->recoveryUnit()->abandonSnapshot(); + if (incrementOpsAppliedStats) { incrementOpsAppliedStats(); } diff --git a/src/mongo/dbtests/storage_timestamp_tests.cpp b/src/mongo/dbtests/storage_timestamp_tests.cpp index d856592ff64..a3b701d51e9 100644 --- a/src/mongo/dbtests/storage_timestamp_tests.cpp +++ b/src/mongo/dbtests/storage_timestamp_tests.cpp @@ -2298,9 +2298,8 @@ public: // Create a background index via `applyOps`. We will timestamp the beginning at // `startBuildTs` and the end, due to manipulation of the logical clock, should be // timestamped at `endBuildTs`. - const auto beforeBuildTime = _clock->reserveTicks(3); + const auto beforeBuildTime = _clock->reserveTicks(2); const auto startBuildTs = beforeBuildTime.addTicks(1).asTimestamp(); - const auto endBuildTs = beforeBuildTime.addTicks(3).asTimestamp(); // Grab the existing idents to identify the ident created by the index build. auto kvStorageEngine = @@ -2339,22 +2338,10 @@ public: assertIdentsMissingAtTimestamp( kvCatalog, "", indexIdent, beforeBuildTime.asTimestamp()); assertIdentsExistAtTimestamp(kvCatalog, "", indexIdent, startBuildTs); - if (Foreground) { - // In the Foreground case, the index build should start and finish at - // `startBuildTs`. - ASSERT_TRUE( - getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, startBuildTs), "field_1") - .ready); - } else { - // In the Background case, the index build should not be "ready" at `startBuildTs`. - ASSERT_FALSE( - getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, startBuildTs), "field_1") - .ready); - assertIdentsExistAtTimestamp(kvCatalog, "", indexIdent, endBuildTs); - ASSERT_TRUE( - getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, endBuildTs), "field_1") - .ready); - } + + // In all cases, the index build should start and finish at `startBuildTs`. + ASSERT_TRUE( + getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, startBuildTs), "field_1").ready); } } }; -- cgit v1.2.1