MB-50320: Explicit deletion obeys CMQuota

Currently CMD_DEL bypasses the checkpoint memory state, thus
potentially spiking checkpoint's mem-usage. This patch prevents that.

Change-Id: Ifc5abd26b186ee8258842776b564283f70e2a73e
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/168815
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>

engines/ep/src/kv_bucket.cc

@@ -2000,6 +2000,11 @@ cb::engine_errc KVBucket::deleteItem(
         return cb::engine_errc::temporary_failure;
     }
 
+    // Yield if checkpoint's full - The call also wakes up the mem recovery task
+    if (verifyCheckpointMemoryState() == CheckpointMemoryState::Full) {
+        return cb::engine_errc::temporary_failure;
+    }
+
     cb::engine_errc result;
     { // collections read scope
         auto cHandle = vb->lockCollections(key);

engines/ep/tests/module_tests/evp_store_single_threaded_test.cc

@@ -5247,4 +5247,47 @@ TEST_P(STParameterizedBucketTest, CheckpointMemThresholdEnforced_ExpiryByRead) {
     };
 
     testExpiryObservesCMQuota(read);
+}
+
+TEST_P(STParameterizedBucketTest, CheckpointMemThresholdEnforced_Del) {
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
+    const auto vb = store->getVBucket(vbid);
+    ASSERT_EQ(0, vb->getHighSeqno());
+
+    // Load and hit the CM Quota
+    ASSERT_EQ(KVBucket::CheckpointMemoryState::Available,
+              store->getCheckpointMemoryState());
+    EXPECT_GT(loadUpToOOM(VbucketOp::Add), 1);
+    EXPECT_EQ(KVBucket::CheckpointMemoryState::Full,
+              store->getCheckpointMemoryState());
+
+    const auto highSeqno = vb->getHighSeqno();
+    ASSERT_GT(highSeqno, 0);
+    const auto& stats = engine->getEpStats();
+    ASSERT_EQ(0, stats.numOpsDelete);
+
+    // Try deleting an item -> TempOOM
+    // First ensure the doc is alive
+    const auto key = makeStoredDocKey("key_" + std::to_string(highSeqno));
+    ASSERT_EQ(cb::engine_errc::success,
+              store->get(key, vbid, nullptr, get_options_t::NONE).getStatus());
+    // Then attempt deletion
+    mutation_descr_t delInfo;
+    uint64_t cas = 0;
+    EXPECT_EQ(cb::engine_errc::temporary_failure,
+              store->deleteItem(key, cas, vbid, cookie, {}, nullptr, delInfo));
+    ASSERT_EQ(0, stats.numOpsDelete);
+
+    // CM memory recovery
+    ASSERT_EQ(KVBucket::CheckpointMemoryState::Full,
+              store->getCheckpointMemoryState());
+    // Release all the releasable from checkpoints
+    flushAndRemoveCheckpoints(vbid);
+    flushAndExpelFromCheckpoints(vbid);
+    EXPECT_EQ(KVBucket::CheckpointMemoryState::Available,
+              store->getCheckpointMemoryState());
+
+    // Now we can delete docs
+    EXPECT_EQ(cb::engine_errc::success,
+              store->deleteItem(key, cas, vbid, cookie, {}, nullptr, delInfo));
 }

engines/ep/tests/module_tests/item_pager_test.cc

@@ -1875,7 +1875,18 @@ TEST_P(STItemPagerTest, MB43055_MemUsedDropDoesNotBreakEviction) {
 
     if (std::get<1>(GetParam()) == "fail_new_data") {
         // items are not auto-deleted, so the ItemPager does not run.
-        return;
+        GTEST_SKIP();
+    }
+
+    // No need to run under nexus:
+    //   - The test has been spotted quite fragile multiple times (eg, magma).
+    //     It makes assumptions on the mem state of the system after persistence
+    //     but that state varies depending on the storage
+    //   - The test covers in-memory behaviour and doesn't care about the
+    //     particular storage used, so we can just keep the couchstore/magma
+    //     versions for persistence, plus ephemeral.
+    if (isNexus()) {
+        GTEST_SKIP();
     }
 
     // Need a slightly higher quota here
@@ -1894,6 +1905,8 @@ TEST_P(STItemPagerTest, MB43055_MemUsedDropDoesNotBreakEviction) {
     auto itemCount = populateUntilAboveHighWaterMark(vbid);
     EXPECT_LT(0, itemCount);
 
+    flushVBucketToDiskIfPersistent(vbid, itemCount);
+
     // now delete some items to lower memory usage
     for (int i = 0; i < itemCount; i++) {
         auto key = makeStoredDocKey("key_" + std::to_string(i));
@@ -1907,19 +1920,22 @@ TEST_P(STItemPagerTest, MB43055_MemUsedDropDoesNotBreakEviction) {
                                     {},
                                     /*itemMeta*/ nullptr,
                                     mutation_descr));
-    }
 
-    // Deleting items doesn't free much memory and we'd need a big quota to be
-    // able to reclaim the difference between low and high watermarks just by
-    // deleting items. We can also flush persistent VBuckets and reduce the
-    // checkpoint memory usage by freeing closed checkpoints. Doing this allows
-    // us to use a significantly lower quota which reduces test time
-    // significantly
-    auto vb = store->getVBucket(vbid);
-    vb->checkpointManager->createNewCheckpoint(true);
-    flushVBucketToDiskIfPersistent(vbid, itemCount);
-    vb->checkpointManager->removeClosedUnrefCheckpoints();
-    runCheckpointDestroyer(vbid);
+        // Deleting items doesn't free much memory and we'd need a big quota to
+        // be able to reclaim the difference between low and high watermarks
+        // just by deleting items. We can also flush persistent VBuckets and
+        // reduce the checkpoint memory usage by freeing closed checkpoints.
+        // Doing this allows us to use a significantly lower quota which reduces
+        // test time significantly.
+        //
+        // Note: Doing this step within the loop for avoiding deletions being
+        // failed by high CM mem-usage
+        auto vb = store->getVBucket(vbid);
+        vb->checkpointManager->createNewCheckpoint(true);
+        flushVBucketToDiskIfPersistent(vbid, 1);
+        vb->checkpointManager->removeClosedUnrefCheckpoints();
+        runCheckpointDestroyer(vbid);
+    }
 
     auto& stats = engine->getEpStats();
     // confirm we are now below the low watermark, and can test the item pager