Resolves #138: Cache should always include collection context with indexes

src/Constants.cpp

@@ -27,6 +27,8 @@ const uint64_t INDEX_KEY_LENGTH_LIMIT = (uint64_t)1e4;
 const uint64_t FDB_KEY_LENGTH_LIMIT = (uint64_t)1e4;
 const uint64_t FDB_VALUE_LENGTH_LIMIT = (uint64_t)1e5;
 
+const uint64_t METADATA_CACHE_SIZE = (uint64_t)100;
+
 const std::string METADATA = "metadata";
 const std::string VERSION_KEY = "version";
 const std::string INDICES_KEY = "indices";

src/Constants.h

@@ -34,6 +34,9 @@ extern const uint64_t INDEX_KEY_LENGTH_LIMIT; // This is set to 10K bytes, inste
 extern const uint64_t FDB_KEY_LENGTH_LIMIT;
 extern const uint64_t FDB_VALUE_LENGTH_LIMIT;
 
+// Size of metadata cache in entries, number of collections
+extern const uint64_t METADATA_CACHE_SIZE;
+
 // KVS DocLayer internal keys
 extern const std::string METADATA;
 extern const std::string VERSION_KEY;

src/ExtCmd.actor.cpp

@@ -50,6 +50,7 @@ ACTOR static Future<std::pair<int, int>> dropIndexMatching(Reference<DocTransact
 	state std::vector<bson::BSONObj> indexes = wait(getIndexesTransactionally(indexesPlan, tr));
 	state int count = 0;
 	state bool any = false;
+	state bson::BSONObj matchingIndexObj;
 	state Reference<QueryContext> matchingIndex;
 	state std::string matchingName;
 
@@ -63,13 +64,15 @@ ACTOR static Future<std::pair<int, int>> dropIndexMatching(Reference<DocTransact
 		if (value.getBSONType() == bson::BSONType::String) {
 			if (value.getString() == indexObj.getStringField(field.c_str())) {
 				any = true;
+				matchingIndexObj = indexObj;
 				matchingIndex = indexesCollection->bindCollectionContext(tr)->cx->getSubContext(
 				    DataValue(indexObj.getField(DocLayerConstants::ID_FIELD)).encode_key_part());
 				matchingName = std::string(indexObj.getStringField(DocLayerConstants::NAME_FIELD));
 			}
 		} else if (value.getBSONType() == bson::BSONType::Object) {
 			if (value.getPackedObject().woCompare(indexObj.getObjectField(field.c_str())) == 0) {
 				any = true;
+				matchingIndexObj = indexObj;
 				matchingIndex = indexesCollection->bindCollectionContext(tr)->cx->getSubContext(
 				    DataValue(indexObj.getField(DocLayerConstants::ID_FIELD)).encode_key_part());
 				matchingName = std::string(indexObj.getStringField(DocLayerConstants::NAME_FIELD));
@@ -82,6 +85,11 @@ ACTOR static Future<std::pair<int, int>> dropIndexMatching(Reference<DocTransact
 	matchingIndex->clearDescendants();
 	matchingIndex->clearRoot();
 
+	TraceEvent(SevInfo, "BumpMetadataVersion")
+	    .detail("reason", "dropIndex")
+	    .detail("ns", fullCollNameToString(ns))
+	    .detail("index", matchingIndexObj.toString());
+
 	Void _ = wait(matchingIndex->commitChanges());
 
 	Key indexKey = targetedCollection->getIndexesSubspace().withSuffix(StringRef(encodeMaybeDotted(matchingName)));
@@ -356,6 +364,9 @@ ACTOR static Future<int> internal_doDropIndexesActor(Reference<DocTransaction> t
 	Key indexes = unbound->getIndexesSubspace();
 	tr->tr->clear(FDB::KeyRangeRef(indexes, strinc(indexes)));
 	unbound->bindCollectionContext(tr)->bumpMetadataVersion();
+	TraceEvent(SevInfo, "BumpMetadataVersion")
+	    .detail("reason", "dropAllIndexes")
+	    .detail("ns", fullCollNameToString(ns));
 
 	return count;
 }

src/ExtMsg.actor.cpp

@@ -63,10 +63,6 @@ Namespace getDBCollectionPair(const char* ns, std::pair<std::string, std::string
 	return std::make_pair(std::string(ns, dotPtr - ns), std::string(dotPtr + 1));
 }
 
-std::string fullCollNameToString(Namespace const& ns) {
-	return ns.first + "." + (ns.second.empty() ? "$cmd" : ns.second);
-}
-
 /**
  * query := { $bool_op : [ query* ],   *          (a predicate)
  * 			  path : literal_match,    *
@@ -1208,8 +1204,7 @@ ACTOR Future<WriteCmdResult> doDeleteCmd(Namespace ns,
 
 		// If collection not found then just return success from here.
 		try {
-			Reference<UnboundCollectionContext> _cx =
-			    wait(ec->mm->getUnboundCollectionContext(dtr, ns, false, true, false));
+			Reference<UnboundCollectionContext> _cx = wait(ec->mm->getUnboundCollectionContext(dtr, ns, false, false));
 			cx = _cx;
 		} catch (Error& e) {
 			if (e.code() == error_code_collection_not_found)

src/MetadataManager.actor.cpp

@@ -25,6 +25,10 @@
 
 using namespace FDB;
 
+std::string fullCollNameToString(Namespace const& ns) {
+	return ns.first + "." + (ns.second.empty() ? "$cmd" : ns.second);
+}
+
 Future<uint64_t> getMetadataVersion(Reference<DocTransaction> tr, Reference<DirectorySubspace> metadataDirectory) {
 	std::string versionKey = metadataDirectory->key().toString() +
 	                         DataValue(DocLayerConstants::VERSION_KEY, DVTypeCode::STRING).encode_key_part();
@@ -82,12 +86,8 @@ IndexInfo MetadataManager::indexInfoFromObj(const bson::BSONObj& indexObj, Refer
 	}
 }
 
-ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>> constructContext(
-    Namespace ns,
-    Reference<DocTransaction> tr,
-    DocumentLayer* docLayer,
-    bool includeIndex,
-    bool createCollectionIfAbsent) {
+ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>>
+constructContext(Namespace ns, Reference<DocTransaction> tr, DocumentLayer* docLayer, bool createCollectionIfAbsent) {
 	try {
 		// The initial set of directory reads take place in a separate transaction with the same read version as `tr'.
 		// This hopefully prevents us from accidentally RYWing a directory that `tr' itself created, and then adding it
@@ -108,20 +108,15 @@ ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>> co
 		state Reference<UnboundCollectionContext> cx =
 		    Reference<UnboundCollectionContext>(new UnboundCollectionContext(collectionDirectory, metadataDirectory));
 
-		// Only include existing indexes into the context when it's NOT building a new index.
-		// When it's building a new index, it's unnecessary and inefficient to pass each recorded returned by a
-		// TableScan through the existing indexes.
-		if (includeIndex) {
-			state Reference<UnboundCollectionContext> indexCx = Reference<UnboundCollectionContext>(
-			    new UnboundCollectionContext(indexDirectory, Reference<DirectorySubspace>()));
-			state Reference<Plan> indexesPlan = getIndexesForCollectionPlan(indexCx, ns);
-			std::vector<bson::BSONObj> allIndexes = wait(getIndexesTransactionally(indexesPlan, tr));
+		state Reference<UnboundCollectionContext> indexCx = Reference<UnboundCollectionContext>(
+		    new UnboundCollectionContext(indexDirectory, Reference<DirectorySubspace>()));
+		state Reference<Plan> indexesPlan = getIndexesForCollectionPlan(indexCx, ns);
+		std::vector<bson::BSONObj> allIndexes = wait(getIndexesTransactionally(indexesPlan, tr));
 
-			for (const auto& indexObj : allIndexes) {
-				IndexInfo index = MetadataManager::indexInfoFromObj(indexObj, cx);
-				if (index.status != IndexInfo::IndexStatus::INVALID) {
-					cx->addIndex(index);
-				}
+		for (const auto& indexObj : allIndexes) {
+			IndexInfo index = MetadataManager::indexInfoFromObj(indexObj, cx);
+			if (index.status != IndexInfo::IndexStatus::INVALID) {
+				cx->addIndex(index);
 			}
 		}
 
@@ -157,6 +152,9 @@ ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>> co
 		// collectionName.c_str(), printable(tcollectionDirectory->key()).c_str(),
 		// printable(tmetadataDirectory->key()).c_str(), "");
 		tcx->bindCollectionContext(tr)->bumpMetadataVersion(); // We start at version 1.
+		TraceEvent(SevInfo, "BumpMetadataVersion")
+		    .detail("reason", "createCollection")
+		    .detail("ns", fullCollNameToString(ns));
 
 		return std::make_pair(tcx, -1); // So we don't pollute the cache in case this transaction never commits
 	}
@@ -165,20 +163,22 @@ ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>> co
 ACTOR static Future<Reference<UnboundCollectionContext>> assembleCollectionContext(Reference<DocTransaction> tr,
                                                                                    Namespace ns,
                                                                                    Reference<MetadataManager> self,
-                                                                                   bool includeIndex,
                                                                                    bool createCollectionIfAbsent) {
-	if (self->contexts.size() > 100)
+	if (self->contexts.size() > DocLayerConstants::METADATA_CACHE_SIZE)
 		self->contexts.clear();
 
 	auto match = self->contexts.find(ns);
 
 	if (match == self->contexts.end()) {
 		std::pair<Reference<UnboundCollectionContext>, uint64_t> unboundPair =
-		    wait(constructContext(ns, tr, self->docLayer, includeIndex, createCollectionIfAbsent));
+		    wait(constructContext(ns, tr, self->docLayer, createCollectionIfAbsent));
 
 		// Here and below don't pollute the cache if we just created the directory, since this transaction might
 		// not commit.
 		if (unboundPair.second != -1) {
+			TraceEvent(SevInfo, "MetadataCacheAdd")
+			    .detail("ns", fullCollNameToString(ns))
+			    .detail("version", unboundPair.second);
 			auto insert_result = self->contexts.insert(std::make_pair(ns, unboundPair));
 			// Somebody else may have done the lookup and finished ahead of us. Either way, replace it with ours (can no
 			// longer optimize this by only replacing if ours is newer, because the directory may have moved or
@@ -194,19 +194,21 @@ ACTOR static Future<Reference<UnboundCollectionContext>> assembleCollectionConte
 		uint64_t version = wait(getMetadataVersion(tr, oldUnbound->metadataDirectory));
 		if (version != oldVersion) {
 			std::pair<Reference<UnboundCollectionContext>, uint64_t> unboundPair =
-			    wait(constructContext(ns, tr, self->docLayer, includeIndex, createCollectionIfAbsent));
+			    wait(constructContext(ns, tr, self->docLayer, createCollectionIfAbsent));
 			if (unboundPair.second != -1) {
 				// Create the iterator again instead of making the previous value state, because the map could have
 				// changed during the previous wait. Either way, replace it with ours (can no longer optimize this by
 				// only replacing if ours is newer, because the directory may have moved or vanished.
-				// std::map<std::pair<std::string, std::string>, std::pair<Reference<UnboundCollectionContext>,
-				// uint64_t>>::iterator match = self->contexts.find(ns);
 				auto match = self->contexts.find(ns);
-
 				if (match != self->contexts.end())
 					match->second = unboundPair;
 				else
 					self->contexts.insert(std::make_pair(ns, unboundPair));
+
+				TraceEvent(SevInfo, "MetadataCacheUpdate")
+				    .detail("ns", fullCollNameToString(ns))
+				    .detail("oldVersion", oldVersion)
+				    .detail("newVersion", unboundPair.second);
 			}
 			return unboundPair.first;
 		} else {
@@ -219,19 +221,17 @@ Future<Reference<UnboundCollectionContext>> MetadataManager::getUnboundCollectio
     Reference<DocTransaction> tr,
     Namespace const& ns,
     bool allowSystemNamespace,
-    bool includeIndex,
     bool createCollectionIfAbsent) {
 	if (!allowSystemNamespace && startsWith(ns.second.c_str(), "system."))
 		throw write_system_namespace();
-	return assembleCollectionContext(tr, ns, Reference<MetadataManager>::addRef(this), includeIndex,
-	                                 createCollectionIfAbsent);
+	return assembleCollectionContext(tr, ns, Reference<MetadataManager>::addRef(this), createCollectionIfAbsent);
 }
 
 Future<Reference<UnboundCollectionContext>> MetadataManager::refreshUnboundCollectionContext(
     Reference<UnboundCollectionContext> cx,
     Reference<DocTransaction> tr) {
 	return assembleCollectionContext(tr, std::make_pair(cx->databaseName(), cx->collectionName()),
-	                                 Reference<MetadataManager>::addRef(this), false, false);
+	                                 Reference<MetadataManager>::addRef(this), false);
 }
 
 ACTOR static Future<Void> buildIndex_impl(bson::BSONObj indexObj,
@@ -242,7 +242,7 @@ ACTOR static Future<Void> buildIndex_impl(bson::BSONObj indexObj,
 	state IndexInfo info;
 	try {
 		state Reference<DocTransaction> tr = ec->getOperationTransaction();
-		state Reference<UnboundCollectionContext> mcx = wait(ec->mm->getUnboundCollectionContext(tr, ns, false, false));
+		state Reference<UnboundCollectionContext> mcx = wait(ec->mm->getUnboundCollectionContext(tr, ns, false));
 		info = MetadataManager::indexInfoFromObj(indexObj, mcx);
 		info.status = IndexInfo::IndexStatus::BUILDING;
 		info.buildId = build_id;

src/MetadataManager.h

@@ -31,14 +31,15 @@
 
 using Namespace = std::pair<std::string, std::string>;
 
+std::string fullCollNameToString(Namespace const& ns);
+
 struct MetadataManager : ReferenceCounted<MetadataManager>, NonCopyable {
 	explicit MetadataManager(struct DocumentLayer* docLayer) : docLayer(docLayer) {}
 	~MetadataManager() = default;
 
 	Future<Reference<UnboundCollectionContext>> getUnboundCollectionContext(Reference<DocTransaction> tr,
 	                                                                        Namespace const& ns,
 	                                                                        bool allowSystemNamespace = false,
-	                                                                        bool includeIndex = true,
 	                                                                        bool createCollectionIfAbsent = true);
 	Future<Reference<UnboundCollectionContext>> refreshUnboundCollectionContext(Reference<UnboundCollectionContext> cx,
 	                                                                            Reference<DocTransaction> tr);

src/QLPlan.actor.cpp

@@ -1418,6 +1418,10 @@ ACTOR static Future<Void> doIndexInsert(PlanCheckpoint* checkpoint,
 
 		Reference<IReadWriteContext> doc = wait(indexInsert->insert(unbound->bindCollectionContext(tr)));
 		mcx->bindCollectionContext(tr)->bumpMetadataVersion();
+		TraceEvent(SevInfo, "BumpMetadataVersion")
+		    .detail("reason", "createIndex")
+		    .detail("ns", fullCollNameToString(ns))
+		    .detail("index", indexObj.toString());
 		output.send(ref(new ScanReturnedContext(doc, -1, Key())));
 		throw end_of_stream();
 	} catch (Error& e) {
@@ -1590,6 +1594,11 @@ ACTOR static Future<Void> updateIndexStatus(PlanCheckpoint* checkpoint,
 			    DataValue(DocLayerConstants::CURRENTLY_PROCESSING_DOC_FIELD, DVTypeCode::STRING).encode_key_part());
 			indexDoc->clear(DataValue(DocLayerConstants::BUILD_ID_FIELD, DVTypeCode::STRING).encode_key_part());
 			mcx->bumpMetadataVersion();
+			TraceEvent(SevInfo, "BumpMetadataVersion")
+			    .detail("reason", "updateIndexStatus")
+			    .detail("ns", fullCollNameToString(ns))
+			    .detail("indexID", printable(encodedIndexId))
+			    .detail("status", newStatus);
 			output.send(ref(new ScanReturnedContext(indexDoc, -1, Key())));
 			throw end_of_stream();
 		} else {