From 0e49b51322ee56ae06507ad189390bb0ab9ee6da Mon Sep 17 00:00:00 2001
From: Ben Huddleston <ben.huddleston@couchbase.com>
Date: Mon, 20 Sep 2021 16:27:32 +0100
Subject: [PATCH] MB-48473: Don't move purge seqno for implictly purged logical
 deletes

We shouldn't move the purge seqno for these items as we only need to
ensure that clients receive the collection end tombstone. Moving
the purge seqno leads to increased and potentially unnecessary rolling
back.

Change-Id: Ifaf680e302fdc37b1f927189a78efd65374ea26a
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/161847
Reviewed-by: Richard de Mellow <richard.demellow@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
---
 .../kvstore/magma-kvstore/magma-kvstore.cc    |  1 -
 .../tests/module_tests/magma_bucket_tests.cc  | 48 ++++++++++++++++++-
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc
index 1bbd195c84..cf1d78eb44 100644
--- a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc
+++ b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc
@@ -301,7 +301,6 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx,
                         "{}",
                         userSanitizedItemStr);
             }
-            maybeUpdatePurgeSeqno();
             return true;
         }
     }
diff --git a/engines/ep/tests/module_tests/magma_bucket_tests.cc b/engines/ep/tests/module_tests/magma_bucket_tests.cc
index c9482a6827..16d19dd93c 100644
--- a/engines/ep/tests/module_tests/magma_bucket_tests.cc
+++ b/engines/ep/tests/module_tests/magma_bucket_tests.cc
@@ -34,9 +34,11 @@ class STParamMagmaBucketTest : public STParamPersistentBucketTest {
      * test the post-compaction state.
      *
      * @param storeItemsForTest Lambda storing items we care about testing
+     * @param postTimeTravelFn Labmda for doing things after the time travel
      */
     void setupForImplicitCompactionTest(
-            std::function<void()> storeItemsForTest);
+            std::function<void()> storeItemsForTest,
+            std::function<void()> postTimeTravelFn = []() {});
 };
 
 /**
@@ -263,7 +265,8 @@ TEST_P(STParamMagmaBucketTest, makeCompactionContextSetupAtWarmup) {
 }
 
 void STParamMagmaBucketTest::setupForImplicitCompactionTest(
-        std::function<void()> storeItemsForTest) {
+        std::function<void()> storeItemsForTest,
+        std::function<void()> postTimeTravelFn) {
     // Function to perform 16 writes so that we hit the LSMMaxNumLevel0Tables
     // threshold which will trigger implicit compaction
     auto performWritesForImplicitCompaction = [this]() -> void {
@@ -302,6 +305,8 @@ void STParamMagmaBucketTest::setupForImplicitCompactionTest(
     // compact
     TimeTraveller timmy{60 * 60 * 24 * 5};
 
+    postTimeTravelFn();
+
     ThreadGate tg(2);
     auto& bucket = dynamic_cast<EPBucket&>(*store);
     bucket.postPurgeSeqnoImplicitCompactionHook = [&tg]() -> void {
@@ -407,6 +412,45 @@ TEST_P(STParamMagmaBucketTest,
     EXPECT_EQ(expectedPurgeSeqno, vb->getPurgeSeqno());
 }
 
+TEST_P(STParamMagmaBucketTest,
+       CheckImplicitCompactionDoesNotUpdatePurgeSeqnoForLogicallyDeletedItem) {
+    uint64_t expectedPurgeSeqno;
+    auto purgedKey = makeStoredDocKey("keyA", CollectionEntry::fruit.getId());
+    CollectionsManifest cm;
+
+    setupForImplicitCompactionTest(
+            [this, &expectedPurgeSeqno, &purgedKey, &cm]() {
+                auto vb = store->getVBucket(vbid);
+                expectedPurgeSeqno = vb->getHighSeqno();
+
+                cm.add(CollectionEntry::fruit);
+                vb->updateFromManifest(makeManifest(cm));
+                store_item(vbid, purgedKey, "value");
+                flushVBucketToDiskIfPersistent(vbid, 2);
+            },
+            [this, &cm]() {
+                // Now remove the collection (which won't get purged as this is
+                // run after the time travel. This can't be purged or it moves
+                // the purge seqno and invalidates the test
+                auto vb = store->getVBucket(vbid);
+                cm.remove(CollectionEntry::fruit);
+                vb->updateFromManifest(makeManifest(cm));
+                flushVBucketToDiskIfPersistent(vbid, 1);
+            });
+
+    auto magmaKVStore =
+            dynamic_cast<MagmaKVStore*>(store->getRWUnderlying(vbid));
+    ASSERT_TRUE(magmaKVStore);
+    // Assert that the collection key no longer has a tomb stone
+    auto gv = magmaKVStore->get(DiskDocKey(purgedKey), vbid);
+    ASSERT_EQ(cb::engine_errc::no_such_key, gv.getStatus());
+
+    // Ensure that the purge seqno has been set during the second flush to where
+    // the first tombstone was
+    auto vb = store->getVBucket(vbid);
+    EXPECT_EQ(expectedPurgeSeqno, vb->getPurgeSeqno());
+}
+
 /**
  * Test that implicit/explicit compaction overlapping handles setting the purge
  * seqno correctly.