MB-54516: Handle a dropped Modify event in backfill

Backfill skips out dropped collection data, the new Modify event must
be included in this.

Change-Id: I1409b6caa42e63983d784ffc020e84ed5bbd259a
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/184088
Well-Formed: Restriction Checker
Tested-by: Jim Walker <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jimwwalker

engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc

@@ -2830,18 +2830,17 @@ static int bySeqnoScanCallback(Db* db, DocInfo* docinfo, void* ctx) {
 
     auto diskKey = makeDiskDocKey(docinfo->id);
 
-    // Determine if the key is logically deleted, if it is we skip the key
-    // Note that system event keys (like create scope) are never skipped here
+    // Determine if the key is logically deleted
     auto docKey = diskKey.getDocKey();
-    if (!docKey.isInSystemCollection()) {
-        if (sctx->docFilter !=
-            DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) {
-            if (sctx->collectionsContext.isLogicallyDeleted(docKey, byseqno)) {
-                sctx->lastReadSeqno = byseqno;
-                return COUCHSTORE_SUCCESS;
-            }
+    if (sctx->docFilter != DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) {
+        if (sctx->collectionsContext.isLogicallyDeleted(docKey, byseqno)) {
+            sctx->lastReadSeqno = byseqno;
+            return COUCHSTORE_SUCCESS;
         }
+    }
 
+    // Only do cache lookup for non-system events
+    if (!docKey.isInSystemCollection()) {
         CacheLookup lookup(diskKey, byseqno, vbucketId);
 
         cl.callback(lookup);

engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc

@@ -1962,29 +1962,25 @@ MagmaScanResult MagmaKVStore::scanOne(
         return MagmaScanResult::Next();
     }
 
-    // Determine if the key is logically deleted, if it is we skip the key
-    // Note that system event keys (like create scope) are never skipped
-    // here
-    if (!lookup.getKey().getDocKey().isInSystemCollection()) {
-        if (ctx.docFilter !=
-            DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) {
-            if (ctx.collectionsContext.isLogicallyDeleted(
-                        lookup.getKey().getDocKey(), seqno)) {
-                ctx.lastReadSeqno = seqno;
-                if (logger->should_log(spdlog::level::TRACE)) {
-                    logger->TRACE(
-                            "MagmaKVStore::scanOne SKIPPED(Collection "
-                            "Deleted) {} "
-                            "key:{} "
-                            "seqno:{}",
-                            ctx.vbid,
-                            cb::UserData{lookup.getKey().to_string()},
-                            seqno);
-                }
-                return MagmaScanResult::Next();
+    // Determine if the key is logically deleted before trying cache/disk read
+    if (ctx.docFilter != DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) {
+        if (ctx.collectionsContext.isLogicallyDeleted(
+                    lookup.getKey().getDocKey(), seqno)) {
+            ctx.lastReadSeqno = seqno;
+            if (logger->should_log(spdlog::level::TRACE)) {
+                logger->TRACE(
+                        "MagmaKVStore::scanOne SKIPPED(Collection "
+                        "Deleted) {} key:{} seqno:{}",
+                        ctx.vbid,
+                        cb::UserData{lookup.getKey().to_string()},
+                        seqno);
             }
+            return MagmaScanResult::Next();
         }
+    }
 
+    // system collections aren't in cache so we can skip that lookup
+    if (!lookup.getKey().getDocKey().isInSystemCollection()) {
         ctx.getCacheCallback().callback(lookup);
         if (ctx.getCacheCallback().getStatus() ==
             static_cast<int>(cb::engine_errc::key_already_exists)) {

engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc

@@ -4338,6 +4338,38 @@ TEST_P(CollectionsDcpPersistentOnly, ModifyCollection) {
               vb1->lockCollections().getCanDeduplicate(fruit));
     EXPECT_EQ(CanDeduplicate::Yes,
               vb1->lockCollections().getCanDeduplicate(vegetable));
+
+    // Final stage of the test - drop one of the modified collections and check
+    // backfill does not play back the modify
+    cm.remove(CollectionEntry::vegetable);
+    setCollections(cookie, cm);
+    flush_vbucket_to_disk(vbid, 1);
+
+    vb0.reset();
+    vb1.reset();
+    resetEngineAndWarmup();
+
+    createDcpObjects({{nullptr, 0}});
+    notifyAndStepToCheckpoint(ClientOpcode::DcpSnapshotMarker, false);
+
+    // Verify that modify vegetable is not transmitted
+    producer->stepAndExpect(*producers, ClientOpcode::DcpSystemEvent);
+    EXPECT_EQ(producers->last_system_event, id::CreateCollection);
+    EXPECT_EQ(producers->last_collection_id, fruit.getId());
+
+    producer->stepAndExpect(*producers, ClientOpcode::DcpSystemEvent);
+    EXPECT_EQ(producers->last_system_event, id::ModifyCollection);
+    EXPECT_EQ(producers->last_collection_id, fruit.getId());
+
+    producer->stepAndExpect(*producers, ClientOpcode::DcpSystemEvent);
+    EXPECT_EQ(producers->last_system_event, id::DeleteCollection);
+    EXPECT_EQ(producers->last_collection_id, vegetable.getId());
+
+    // Reacquire replica and check the backfill events
+    vb1 = store->getVBucket(replicaVB);
+    // replica received modified state
+    EXPECT_EQ(CanDeduplicate::No,
+              vb1->lockCollections().getCanDeduplicate(fruit));
 }
 
 // Test that if the flatBuffesrSystemEventsEnabled==false a modification event