MB-23906: Delete-with-value: Fix count of deleted items 20/76120/15
authorDave Rigby <daver@couchbase.com>
Fri, 31 Mar 2017 10:46:13 +0000 (11:46 +0100)
committerDave Rigby <daver@couchbase.com>
Wed, 19 Apr 2017 07:35:26 +0000 (07:35 +0000)
Prior to Spock, the valid state transitions for StoredValues (relating
to deletion) were:

    /---\
    |   |
    |   V
    Alive --------> Deleted
          <--------

With the advent of Deleted Bodies for system XATTRs, Deleted SVs can
optionally have a value (Blob) associated with them, which can be
changed (or removed) without going back to Alive:

    /---\              /---\
    |   |              |   |
    \   V              \   V
    Alive -------->   Deleted   ----------> Deleted
      ^   <--------  with-value <---------- no-value
      |                                         |
      |                                         |
      \-----------------------------------------/

Given that Deleted-with-Value and Alive both are moved-to using
updateStoredValue(), we need to handle increasing numDeletedItems in
additional code paths - specifically whenever the value changes.

Fix this in HashTable::unlocked_updateStoredValue, and add tests for
these cases to the Delete-with-value tests.

Change-Id: Id9224ed0e66d0298c0f73621fedba3af1fe62e86
Reviewed-on: http://review.couchbase.org/76120
Reviewed-by: Manu Dhundi <manu@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
src/hash_table.cc
tests/ep_testsuite_basic.cc
tests/module_tests/vbucket_test.cc

index 5c2edd9..aa7c141 100644 (file)
@@ -291,6 +291,9 @@ MutationStatus HashTable::unlocked_updateStoredValue(
     if (v.isDeleted() && !itm.isDeleted()) {
         --numDeletedItems;
     }
+    if (!v.isDeleted() && itm.isDeleted()) {
+        ++numDeletedItems;
+    }
 
     // If the item we are replacing is resident then we need to make sure we
     // appropriately alter the datatype stats.
index 7cd02c5..11a37d0 100644 (file)
@@ -1366,6 +1366,10 @@ static enum test_result test_delete_with_value(ENGINE_HANDLE* h,
             delete_with_value(h, h1, cookie, cas_0, "key", "deleted"),
             "Failed Alive -> Delete-with-value");
 
+    checkeq(uint64_t(0),
+            get_stat<uint64_t>(h, h1, "vb_0:num_items", "vbucket-details 0"),
+            "Unexpected num_items after Alive -> Delete-with-value");
+
     auto res = get_value(h, h1, cookie, "key", vbid, DocStateFilter::Alive);
     checkeq(ENGINE_KEY_ENOENT,
             res.first,
@@ -1382,6 +1386,11 @@ static enum test_result test_delete_with_value(ENGINE_HANDLE* h,
             delete_with_value(h, h1, cookie, cas_0, "key", "deleted 2"),
             "Failed Deleted-with-value -> Deleted-with-value");
 
+    checkeq(uint64_t(0),
+            get_stat<uint64_t>(h, h1, "vb_0:num_items", "vbucket-details 0"),
+            "Unexpected num_items after Delete-with-value -> "
+            "Delete-with-value");
+
     res = get_value(h, h1, cookie, "key", vbid, DocStateFilter::AliveOrDeleted);
     checkeq(ENGINE_SUCCESS, res.first, "Failed to fetch key (deleted 2)");
     checkeq(std::string("deleted 2"),
@@ -1394,6 +1403,10 @@ static enum test_result test_delete_with_value(ENGINE_HANDLE* h,
             "Failed Delete-with-value -> Alive");
     wait_for_flusher_to_settle(h, h1);
 
+    checkeq(uint64_t(1),
+            get_stat<uint64_t>(h, h1, "vb_0:num_items", "vbucket-details 0"),
+            "Unexpected num_items after Delete-with-value -> Alive");
+
     res = get_value(h, h1, cookie, "key", vbid, DocStateFilter::Alive);
     checkeq(ENGINE_SUCCESS,
             res.first,
@@ -1415,6 +1428,10 @@ static enum test_result test_delete_with_value(ENGINE_HANDLE* h,
             "Failed Alive -> Deleted-no-value");
     wait_for_flusher_to_settle(h, h1);
 
+    checkeq(uint64_t(0),
+            get_stat<uint64_t>(h, h1, "vb_0:num_items", "vbucket-details 0"),
+            "Unexpected num_items after Alive -> Delete-no-value");
+
     res = get_value(h, h1, cookie, "key", vbid, DocStateFilter::Alive);
     checkeq(ENGINE_KEY_ENOENT,
             res.first,
index 9d92949..a23847d 100644 (file)
@@ -363,6 +363,8 @@ TEST_P(VBucketTest, updateDeletedItem) {
     EXPECT_EQ(MutationStatus::WasDirty,
               this->public_processSoftDelete(v->getKey(), v, 0));
     verifyValue(key, nullptr, TrackReference::Yes, WantsDeleted::Yes);
+    EXPECT_EQ(1, this->vbucket->getNumItems());
+    EXPECT_EQ(1, this->vbucket->getNumInMemoryDeletes());
 
     Item deleted_item(key, 0, 0, "deletedvalue", strlen("deletedvalue"));
     deleted_item.setDeleted();
@@ -377,7 +379,8 @@ TEST_P(VBucketTest, updateDeletedItem) {
                 "deletedvalue",
                 TrackReference::Yes,
                 WantsDeleted::Yes);
-
+    EXPECT_EQ(1, this->vbucket->getNumItems());
+    EXPECT_EQ(1, this->vbucket->getNumInMemoryDeletes());
     EXPECT_EQ(prev_revseqno + 1, v->getRevSeqno());
 
     Item update_deleted_item(
@@ -391,6 +394,8 @@ TEST_P(VBucketTest, updateDeletedItem) {
               this->public_processSet(update_deleted_item, 0));
     verifyValue(
             key, "updatedeletedvalue", TrackReference::Yes, WantsDeleted::Yes);
+    EXPECT_EQ(1, this->vbucket->getNumItems());
+    EXPECT_EQ(1, this->vbucket->getNumInMemoryDeletes());
     EXPECT_EQ(prev_revseqno + 1, v->getRevSeqno());
 }