From 36635c8c4bb018b4e677c331b9c608564ad37301 Mon Sep 17 00:00:00 2001 From: Jordi Olivares Provencio Date: Fri, 2 Aug 2024 14:12:05 +0200 Subject: [PATCH] SERVER-93112 Partially revert SERVER-81890 to allow building indexes with simple and non-simple collations on the same key (#25638) GitOrigin-RevId: 8cdd40c017bca323d68e97f6fab06aaa4b0f60db --- .../suites/v1index_jscore_passthrough.yml | 1 + ...kports_required_for_multiversion_tests.yml | 4 + .../index_creation_on_different_collations.js | 52 +++++++++++++ .../initial_sync_index_conflict_recreate.js | 74 ------------------- src/mongo/db/catalog/index_catalog_impl.cpp | 27 +++---- 5 files changed, 67 insertions(+), 91 deletions(-) create mode 100644 jstests/core/index/index_creation_on_different_collations.js delete mode 100644 jstests/replsets/initial_sync_index_conflict_recreate.js diff --git a/buildscripts/resmokeconfig/suites/v1index_jscore_passthrough.yml b/buildscripts/resmokeconfig/suites/v1index_jscore_passthrough.yml index 75b0510d0d5..f3acb8b736c 100644 --- a/buildscripts/resmokeconfig/suites/v1index_jscore_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/v1index_jscore_passthrough.yml @@ -16,6 +16,7 @@ selector: - jstests/core/index/wildcard/**/*.js # v:1 index does not support collation. - jstests/core/collation.js + - jstests/core/index/index_creation_on_different_collations.js - jstests/core/query/expr/expr_index_use.js # Timeseries collections are clustered, which use v:2 indexes. - jstests/core/timeseries/**/*.js diff --git a/etc/backports_required_for_multiversion_tests.yml b/etc/backports_required_for_multiversion_tests.yml index 606b37d6fe4..f595a8abb10 100644 --- a/etc/backports_required_for_multiversion_tests.yml +++ b/etc/backports_required_for_multiversion_tests.yml @@ -219,6 +219,8 @@ last-continuous: ticket: SERVER-92930 - test_file: jstests/core/query/plan_cache/elem_match_index_diff_types.js ticket: SERVER-92806 + - test_file: jstests/core/index/index_creation_on_different_collations.js + ticket: SERVER-93112 suites: null last-lts: all: @@ -450,4 +452,6 @@ last-lts: ticket: SERVER-92930 - test_file: jstests/core/query/plan_cache/elem_match_index_diff_types.js ticket: SERVER-92806 + - test_file: jstests/core/index/index_creation_on_different_collations.js + ticket: SERVER-93112 suites: null diff --git a/jstests/core/index/index_creation_on_different_collations.js b/jstests/core/index/index_creation_on_different_collations.js new file mode 100644 index 00000000000..696a7807f9f --- /dev/null +++ b/jstests/core/index/index_creation_on_different_collations.js @@ -0,0 +1,52 @@ +/** + * This test ensures that creating an index on a collection with non-simple collation always + * respects the collation option used. This is required for sharding since the sharding index has to + * use the simple collation regardless of the collection's default collation. + * + * @tags: [ + * # We do not expect there to be an existing collection as it can conflict with the + * # createCollection call with a "collection already exists with different options" error. + * assumes_no_implicit_collection_creation_after_drop + * ] + */ + +import {IndexCatalogHelpers} from "jstests/libs/index_catalog_helpers.js"; + +const coll = db.index_creation_on_different_collations; +coll.drop(); + +const nonSimpleCollationOptions = { + locale: "en_US", + strength: 2, + caseLevel: false +}; + +assert.commandWorked(db.createCollection(coll.getName(), {collation: nonSimpleCollationOptions})); + +// Create an index that doesn't specify the collation option. This should use the default value. +assert.commandWorked( + db.runCommand({createIndexes: coll.getName(), indexes: [{key: {x: 1}, name: "x_1"}]})); + +assert.commandWorked(db.runCommand({ + createIndexes: coll.getName(), + indexes: [{key: {x: 1}, name: "x_1_1", collation: {locale: 'simple'}}] +})); + +const indexesFound = coll.getIndexes(); +assert.neq(null, + IndexCatalogHelpers.findByKeyPattern(indexesFound, {x: 1}, {locale: "simple"}), + "Failed to find simple collation index for {x: 1} / Found: " + tojson(indexesFound)); +assert.neq(null, + IndexCatalogHelpers.findByKeyPattern(indexesFound, {x: 1}, { + locale: "en_US", + caseLevel: false, + caseFirst: "off", + strength: 2, + numericOrdering: false, + alternate: "non-ignorable", + maxVariable: "punct", + normalization: false, + backwards: false, + version: "57.1" + }), + "Failed to find complex collation index for {x: 1} / Found: " + tojson(indexesFound)); diff --git a/jstests/replsets/initial_sync_index_conflict_recreate.js b/jstests/replsets/initial_sync_index_conflict_recreate.js deleted file mode 100644 index 96ea228b962..00000000000 --- a/jstests/replsets/initial_sync_index_conflict_recreate.js +++ /dev/null @@ -1,74 +0,0 @@ -/** - * Test that a collection cloned during an index build doesn't cause issues with concurrent index - * operations referring to the same index. - */ -import {configureFailPoint} from "jstests/libs/fail_point_util.js"; -import {IndexBuildTest} from "jstests/noPassthrough/libs/index_build.js"; - -const dbName = 'test'; -const collectionName = 'coll'; - -const times = [ - ISODate('1970-01-01T00:00:00'), - ISODate('1970-01-01T00:00:07'), -]; -let docs = []; -for (const m of ['a', 'A', 'b', 'B']) - for (const t of times) - docs.push({t, m}); - -// Start one-node repl-set. -const rst = new ReplSetTest({nodes: 1}); -rst.startSet(); -rst.initiate(); -const primary = rst.getPrimary(); -const primaryDB = primary.getDB(dbName); - -// Add a secondary. -const secondary = rst.add({ - rsConfig: {votes: 0, priority: 0}, - setParameter: {numInitialSyncAttempts: 1, logComponentVerbosity: '{verbosity: 1}'}, -}); - -// Make the secondary pause once it has scanned the source's oplog. This will cause the secondary to -// apply all oplog entries from now on. -const failPoint = configureFailPoint(secondary, 'initialSyncHangBeforeCopyingDatabases'); -rst.reInitiate(); -failPoint.wait(); - -// The secondary has established the replay oplog begin timestamp. Proceed to create a collection. -primaryDB.createCollection(collectionName, { - collation: {locale: 'en_US', strength: 2}, -}); - -const primaryColl = primaryDB.getCollection(collectionName); -// This will create a createIndexes oplog entry. -primaryColl.createIndex({m: 1, t: 1}, { - collation: {locale: 'simple'}, -}); -// Insert some documents so that another createIndexes becomes a two-phase index build. -assert.commandWorked(primaryColl.insert(docs)); - -// We will now drop all indexes and recreate them. We will also prevent the primary from completing -// the index build. This will cause the secondary to detect an unfinished index build during the -// cloning phase. -primaryColl.dropIndexes(); -IndexBuildTest.pauseIndexBuilds(primary); -const createIdx = IndexBuildTest.startIndexBuild(primary, primaryColl.getFullName(), {m: 1, t: 1}, { - collation: {locale: 'simple'}, -}); -IndexBuildTest.waitForIndexBuildToStart(primaryDB, primaryColl.getName(), "m_1_t_1"); - -// Continue cloning, this will make the unfinished index build be processed by the CollectionCloner -// on the secondary. Pause the secondary before performing oplog replay. -const oplogApplierFp = configureFailPoint(secondary, 'initialSyncHangAfterDataCloning'); -failPoint.off(); -oplogApplierFp.wait(); -// Complete the index build. -IndexBuildTest.resumeIndexBuilds(primary); -oplogApplierFp.off(); -// There are now an unfinished index build + createIndex oplog entry on the secondary. Both entries -// refer to the same index, wait until it's caught up. -rst.awaitReplication(); -createIdx(); -rst.stopSet(); diff --git a/src/mongo/db/catalog/index_catalog_impl.cpp b/src/mongo/db/catalog/index_catalog_impl.cpp index 979263904e1..f5d6838bfa5 100644 --- a/src/mongo/db/catalog/index_catalog_impl.cpp +++ b/src/mongo/db/catalog/index_catalog_impl.cpp @@ -511,18 +511,16 @@ StatusWith IndexCatalogImpl::prepareSpecForCreate( auto validatedSpec = swValidatedAndFixed.getValue(); - // Normalise the spec - const auto normalSpec = IndexCatalog::normalizeIndexSpecs(opCtx, collection, validatedSpec); - // Check whether this is a non-_id index and there are any settings disallowing this server // from building non-_id indexes. - Status status = _isNonIDIndexAndNotAllowedToBuild(opCtx, normalSpec); + Status status = _isNonIDIndexAndNotAllowedToBuild(opCtx, validatedSpec); if (!status.isOK()) { return status; } // First check against only the ready indexes for conflicts. - status = _doesSpecConflictWithExisting(opCtx, collection, normalSpec, InclusionPolicy::kReady); + status = + _doesSpecConflictWithExisting(opCtx, collection, validatedSpec, InclusionPolicy::kReady); if (!status.isOK()) { return status; } @@ -540,7 +538,7 @@ StatusWith IndexCatalogImpl::prepareSpecForCreate( // checking against all indexes occurred due to an in-progress index. status = _doesSpecConflictWithExisting(opCtx, collection, - normalSpec, + validatedSpec, IndexCatalog::InclusionPolicy::kReady | IndexCatalog::InclusionPolicy::kUnfinished | IndexCatalog::InclusionPolicy::kFrozen); @@ -572,14 +570,12 @@ std::vector IndexCatalogImpl::removeExistingIndexesNoChecks( // _doesSpecConflictWithExisting currently does more work than we require here: we are only // interested in the index already exists error. - // Normalise the spec - const auto normalSpec = IndexCatalog::normalizeIndexSpecs(opCtx, collection, spec); auto inclusionPolicy = removeInProgressIndexBuilds ? IndexCatalog::InclusionPolicy::kReady | IndexCatalog::InclusionPolicy::kUnfinished : IndexCatalog::InclusionPolicy::kReady; if (ErrorCodes::IndexAlreadyExists == - _doesSpecConflictWithExisting(opCtx, collection, normalSpec, inclusionPolicy)) { + _doesSpecConflictWithExisting(opCtx, collection, spec, inclusionPolicy)) { continue; } @@ -1103,11 +1099,9 @@ Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx, if (desc) { // Index already exists with same name. Check whether the options are the same as well. - auto normalizedEntry = getEntry(desc)->getNormalizedEntry(opCtx, collection); - + auto entry = getEntry(desc); IndexDescriptor candidate(_getAccessMethodName(key), spec); - auto indexComparison = - candidate.compareIndexOptions(opCtx, collection->ns(), normalizedEntry.get()); + auto indexComparison = candidate.compareIndexOptions(opCtx, collection->ns(), entry); // Key pattern or another uniquely-identifying option differs. We can build this index, // but not with the specified (duplicate) name. User must specify another index name. @@ -1135,7 +1129,7 @@ Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx, // If an identical index exists, but it is frozen, return an error with a different // error code to the user, forcing the user to drop before recreating the index. - if (normalizedEntry->isFrozen()) { + if (entry->isFrozen()) { return Status(ErrorCodes::CannotCreateIndex, str::stream() << "An identical, unfinished index '" << name @@ -1165,11 +1159,10 @@ Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx, // Index already exists with a different name. Check whether the options are identical. // We will return an error in either case, but this check allows us to generate a more // informative error message. - auto normalizedEntry = getEntry(desc)->getNormalizedEntry(opCtx, collection); + auto entry = getEntry(desc); IndexDescriptor candidate(_getAccessMethodName(key), spec); - auto indexComparison = - candidate.compareIndexOptions(opCtx, collection->ns(), normalizedEntry.get()); + auto indexComparison = candidate.compareIndexOptions(opCtx, collection->ns(), entry); // The candidate's key and uniquely-identifying options are equivalent to an existing // index, but some other options are not identical. Return a message to that effect.