MB-42568: [Ephe] Correctly update num-deleted-item in updateStoredValue

In EphemeralVBucket::updateStoredValue we try to update an existing OSV
and move it to the end of the SeqList if possible.
That may be prevented by range-reads that cover that OSV. In that case
we add a new stored value at the end of the SeqList and leave the old
OSV at its place.

Before this patch we used to always update the SeqList::numDeletedItems
counter based on the semantic of an "update SV", also for when the "add
SV" logic is executed.

That led to over-decrementing the num of deletions, which underflows the
counter in some scenarios.

Change-Id: Ib5c8910d68a4edcc0da1bf16e62e0acb51c70894
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/143512
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: paolococchi

engines/ep/src/ephemeral_vb.cc

@@ -437,6 +437,9 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
             auto result = ht.unlocked_updateStoredValue(hbl, v, itm);
             status = result.status;
             newSv = result.storedValue;
+
+            seqList->updateNumDeletedItems(oldValueDeleted, itm.isDeleted());
+
         } break;
 
         case SequenceList::UpdateStatus::Append: {
@@ -456,6 +459,11 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
 
             seqList->appendToList(
                     lh, listWriteLg, *(newSv->toOrderedStoredValue()));
+
+            // We couldn't overwrite any existing OVS due to a range-read, we
+            // just update with the same logic of an "add new stored value"
+            seqList->updateNumDeletedItems(false, itm.isDeleted());
+
         } break;
         }
 
@@ -497,8 +505,6 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
         }
     }
 
-    seqList->updateNumDeletedItems(oldValueDeleted, itm.isDeleted());
-
     return std::make_tuple(newSv, status, notifyCtx);
 }
 

engines/ep/src/ephemeral_vb.h

@@ -280,6 +280,14 @@ class EphemeralVBucket : public VBucket {
         return {};
     }
 
+    size_t getSeqListNumItems() const {
+        return seqList->getNumItems();
+    }
+
+    size_t getSeqListNumDeletedItems() const {
+        return seqList->getNumDeletedItems();
+    }
+
 protected:
     /* Data structure for in-memory sequential storage */
     std::unique_ptr<SequenceList> seqList;

engines/ep/tests/module_tests/ephemeral_bucket_test.cc

@@ -745,3 +745,81 @@ TEST_F(SingleThreadedEphemeralPurgerTest, HTCleanerSkipsPrepares) {
         ASSERT_FALSE(res.committed);
     }
 }
+
+TEST_F(SingleThreadedEphemeralPurgerTest, MB_42568) {
+    // Test relies on that the HTCleaner does its work when it runs
+    ASSERT_EQ(0, engine->getConfiguration().getEphemeralMetadataPurgeAge());
+
+    auto& vb = dynamic_cast<EphemeralVBucket&>(*store->getVBucket(vbid));
+    ASSERT_EQ(0, vb.getHighSeqno());
+    ASSERT_EQ(0, vb.getSeqListNumItems());
+    ASSERT_EQ(0, vb.getSeqListNumDeletedItems());
+
+    const auto runHTCleaner = [this]() -> void {
+        auto* bucket = dynamic_cast<EphemeralBucket*>(store);
+        bucket->enableTombstonePurgerTask();
+        auto& queue = *task_executor->getLpTaskQ()[NONIO_TASK_IDX];
+        bucket->attemptToFreeMemory(); // This wakes up the HTCleaner
+        runNextTask(queue, "Eph tombstone hashtable cleaner");
+        // Scheduled by HTCleaner
+        runNextTask(queue, "Eph tombstone stale item deleter");
+    };
+
+    // keyA - SyncDelete and Commit
+    const auto keyA = makeStoredDocKey("keyA");
+    const std::string value = "value";
+    store_item(vbid,
+               keyA,
+               value,
+               0 /*exptime*/,
+               {cb::engine_errc::sync_write_pending},
+               PROTOCOL_BINARY_RAW_BYTES,
+               cb::durability::Requirements(),
+               true /*deleted*/);
+    EXPECT_EQ(1, vb.getHighSeqno());
+    EXPECT_EQ(1, vb.getSeqListNumItems());
+    EXPECT_EQ(1, vb.getSeqListNumDeletedItems());
+    EXPECT_EQ(
+            ENGINE_SUCCESS,
+            vb.commit(keyA, 1 /*prepareSeqno*/, {}, vb.lockCollections(keyA)));
+    EXPECT_EQ(2, vb.getHighSeqno());
+    EXPECT_EQ(2, vb.getSeqListNumItems());
+    EXPECT_EQ(2, vb.getSeqListNumDeletedItems());
+
+    // Cover seqno 1 and 2 with a range-read, then queue a SyncWrite for keyA.
+    // Seqno 1 is PrepareCommitted and we could just move that OSV to the end of
+    // the SeqList and update it to store the new Pending. But we cannot touch
+    // seqno:1 because of the range-read, so we append a new OSV to the SeqList.
+    {
+        const auto rangeIt = vb.makeRangeIterator(true /*isBackfill*/);
+        ASSERT_TRUE(rangeIt);
+
+        // keyA - SyncWrite
+        store_item(vbid,
+                   keyA,
+                   value,
+                   0 /*exptime*/,
+                   {cb::engine_errc::sync_write_pending},
+                   PROTOCOL_BINARY_RAW_BYTES,
+                   cb::durability::Requirements(),
+                   false /*deleted*/);
+    }
+    EXPECT_EQ(3, vb.getHighSeqno());
+    // Note: This would be 2 with no range-read
+    EXPECT_EQ(3, vb.getSeqListNumItems());
+
+    // Core check: We have not updated the deleted at seqno:1 with the alive at
+    // seqno:3, we have just appended seqno:3. So, num-deleted-items must not
+    // change. Before the fix this was decremented to 1.
+    EXPECT_EQ(2, vb.getSeqListNumDeletedItems());
+
+    // As a side effect, before the fix this step throws with
+    // ThrowExceptionUnderflowPolicy, as we try to remove seqno 1 and 2 (ie,
+    // (two deleted items) from the SeqList and we try to decrement
+    // num-deleted-items to -1.
+    runHTCleaner();
+
+    EXPECT_EQ(3, vb.getHighSeqno());
+    EXPECT_EQ(1, vb.getSeqListNumItems());
+    EXPECT_EQ(0, vb.getSeqListNumDeletedItems());
+}