MB-23711: [Ephemeral] Allow pageOut of Deleted-with-value document

A Deleted-with-value should be able to be paged out under Ephemeral
buckets (if we're low on space we should be able to remove the deleted
body), however this currently fails as it is not permitted to delete
an item which is already marked as deleted.

Given the semantics of Deleted documents are slightly different now we
have Deleted Bodies, we /should/ be able to delete something which has
a deleted value.

Update StoredValue and HashTable to correctly set the deleted flag in
this case, and correct the count of items in the HashTable.

Change-Id: I9bba6fb5779b82b16fa0a6b3bac7ccf468c4c47f
Reviewed-on: http://review.couchbase.org/76128
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>

diff --git a/src/hash_table.cc b/src/hash_table.cc
index 9acd61a..5e8c400 100644 (file)
--- a/src/hash_table.cc
+++ b/src/hash_table.cc
@@ -330,6 +330,9 @@ StoredValue* HashTable::unlocked_addNewStoredValue(const HashBucketLock& hbl,
         ++numTotalItems;
         ++datatypeCounts[v->getDatatype()];
     }
+    if (v->isDeleted()) {
+        ++numDeletedItems;
+    }
     values[hbl.getBucketNum()] = std::move(v);
 
     return values[hbl.getBucketNum()].get();
@@ -370,6 +373,7 @@ HashTable::unlocked_replaceByCopy(const HashBucketLock& hbl,
 void HashTable::unlocked_softDelete(const std::unique_lock<std::mutex>& htLock,
                                     StoredValue& v,
                                     bool onlyMarkDeleted) {
+    const bool alreadyDeleted = v.isDeleted();
     if (!v.isResident() && !v.isDeleted() && !v.isTempItem()) {
         decrNumNonResidentItems();
     }
@@ -386,7 +390,9 @@ void HashTable::unlocked_softDelete(const std::unique_lock<std::mutex>& htLock,
         }
         v.del(*this);
     }
-    ++numDeletedItems;
+    if (!alreadyDeleted) {
+        ++numDeletedItems;
+    }
 }
 
 StoredValue* HashTable::unlocked_find(const DocKey& key,

diff --git a/src/stored-value.cc b/src/stored-value.cc
index ffcddb3..26d501d 100644 (file)
--- a/src/stored-value.cc
+++ b/src/stored-value.cc
@@ -259,12 +259,16 @@ bool StoredValue::operator==(const StoredValue& other) const {
 }
 
 void StoredValue::deleteImpl(HashTable& ht) {
-    if (isDeleted()) {
+    if (isDeleted() && !getValue()) {
+        // SV is already marked as deleted and has no value - no further
+        // deletion possible.
         return;
     }
 
     reduceCacheSize(ht, valuelen());
-    resetValue();
+    markNotResident();
+    // item no longer resident once value is reset
+    deleted = true;
     markDirty();
 }
 

diff --git a/src/stored-value.h b/src/stored-value.h
index c5ccdb4..3c30fa9 100644 (file)
--- a/src/stored-value.h
+++ b/src/stored-value.h
@@ -259,19 +259,6 @@ public:
     }
 
     /**
-     * Reset the value of this item.
-     */
-    void resetValue() {
-        if (isDeleted()) {
-            throw std::logic_error("StoredValue::resetValue: Not possible to "
-                    "reset the value of a deleted item");
-        }
-        markNotResident();
-        // item no longer resident once reset the value
-        deleted = true;
-    }
-
-    /**
      * Eject an item value from memory.
      * @param ht the hashtable that contains this StoredValue instance
      */

diff --git a/src/vbucket.h b/src/vbucket.h
index 6dbf767..5669bbc 100644 (file)
--- a/src/vbucket.h
+++ b/src/vbucket.h
@@ -554,6 +554,13 @@ public:
 
     virtual KVShard* getShard() = 0;
 
+    /**
+     * Returns the number of alive (non-deleted) Items the VBucket.
+     *
+     * Includes items which are not currently resident in memory (i.e. under
+     * Full eviction and have been fully evicted from memory).
+     * Does *not* include deleted items.
+     */
     virtual size_t getNumItems() const = 0;
 
     size_t getNumNonResidentItems() const;

diff --git a/tests/module_tests/ephemeral_vb_test.cc b/tests/module_tests/ephemeral_vb_test.cc
index 62ee806..ef50ec0 100644 (file)
--- a/tests/module_tests/ephemeral_vb_test.cc
+++ b/tests/module_tests/ephemeral_vb_test.cc
@@ -85,6 +85,32 @@ TEST_F(EphemeralVBucketTest, DoublePageOut) {
     EXPECT_TRUE(storedVal->isDeleted());
 }
 
+
+// Verify that we can pageOut deleted items which have a value associated with
+// them - and afterwards the value is null.
+TEST_F(EphemeralVBucketTest, PageOutAfterDeleteWithValue) {
+    // Add an item which is marked as deleted, but has a body (e.g. system
+    // XATTR).
+    auto key = makeStoredDocKey("key");
+    std::string value = "deleted value";
+    Item item(key, 0, /*expiry*/0, value.data(), value.size());
+    item.setDeleted();
+    ASSERT_EQ(AddStatus::Success, public_processAdd(item));
+    ASSERT_EQ(0, vbucket->getNumItems());
+
+    // Check preconditions
+    auto lock_sv = lockAndFind(key);
+    auto* storedVal = lock_sv.second;
+    ASSERT_TRUE(storedVal->isDeleted());
+    ASSERT_EQ(value, storedVal->getValue()->to_s());
+
+    // Page it out.
+    EXPECT_TRUE(vbucket->pageOut(lock_sv.first, storedVal));
+    EXPECT_EQ(0, vbucket->getNumItems());
+    EXPECT_TRUE(storedVal->isDeleted());
+    EXPECT_FALSE(storedVal->getValue());
+}
+
 TEST_F(EphemeralVBucketTest, SetItems) {
     const int numItems = 3;