MB-23994: Update deletedTime when deleted SV is re-deleted 88/76488/11
authorDave Rigby <daver@couchbase.com>
Fri, 7 Apr 2017 14:11:39 +0000 (15:11 +0100)
committerManu Dhundi <manu@couchbase.com>
Tue, 25 Apr 2017 15:57:28 +0000 (15:57 +0000)
When re-deleting an already deleted item (for example when the deleted
item has a body and that body is changed), the DeletedTime was not
updated.  As such, it may be purged too early - the purge age should
be based on the last modification of the item.

Fix by ensuring the deletedTime is updated whenever the item is
(re)deleted, not just the first time.

Change-Id: I8d901cd82720597235f1400dcb2c88643ff7ed10
Reviewed-on: http://review.couchbase.org/76488
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>
src/stored-value.cc
src/stored-value.h
tests/module_tests/ephemeral_vb_test.cc

index 26d501d..b6b59c8 100644 (file)
@@ -29,33 +29,11 @@ const int64_t StoredValue::state_temp_init = -5;
 const int64_t StoredValue::state_collection_open = -6;
 
 void StoredValue::setValue(const Item& itm, HashTable& ht) {
-    size_t currSize = size();
-    reduceCacheSize(ht, currSize);
-    value = itm.getValue();
-    deleted = itm.isDeleted();
-    flags = itm.getFlags();
-    datatype = itm.getDataType();
-    bySeqno = itm.getBySeqno();
-
-    cas = itm.getCas();
-    lock_expiry_or_delete_time = 0;
-    exptime = itm.getExptime();
-    revSeqno = itm.getRevSeqno();
-
-    nru = itm.getNRUValue();
-
-    if (isTempInitialItem()) {
-        markClean();
+    if (isOrdered) {
+        return static_cast<OrderedStoredValue*>(this)->setValueImpl(itm, ht);
     } else {
-        markDirty();
-    }
-
-    if (isTempItem()) {
-        markNotResident();
+        return this->setValueImpl(itm, ht);
     }
-
-    size_t newSize = size();
-    increaseCacheSize(ht, newSize);
 }
 
 bool StoredValue::ejectValue(HashTable &ht, item_eviction_policy_t policy) {
@@ -272,6 +250,36 @@ void StoredValue::deleteImpl(HashTable& ht) {
     markDirty();
 }
 
+void StoredValue::setValueImpl(const Item& itm, HashTable& ht) {
+    size_t currSize = size();
+    reduceCacheSize(ht, currSize);
+    value = itm.getValue();
+    deleted = itm.isDeleted();
+    flags = itm.getFlags();
+    datatype = itm.getDataType();
+    bySeqno = itm.getBySeqno();
+
+    cas = itm.getCas();
+    lock_expiry_or_delete_time = 0;
+    exptime = itm.getExptime();
+    revSeqno = itm.getRevSeqno();
+
+    nru = itm.getNRUValue();
+
+    if (isTempInitialItem()) {
+        markClean();
+    } else {
+        markDirty();
+    }
+
+    if (isTempItem()) {
+        markNotResident();
+    }
+
+    size_t newSize = size();
+    increaseCacheSize(ht, newSize);
+}
+
 std::ostream& operator<<(std::ostream& os, const StoredValue& sv) {
 
     // type, address
@@ -342,6 +350,16 @@ void OrderedStoredValue::deleteImpl(HashTable& ht) {
     setDeletedTime(ep_current_time());
 }
 
+void OrderedStoredValue::setValueImpl(const Item& itm, HashTable& ht) {
+    StoredValue::setValueImpl(itm, ht);
+
+    // Update the deleted time (note - even if it was already deleted we should
+    // refresh this).
+    if (isDeleted()) {
+        setDeletedTime(ep_current_time());
+    }
+}
+
 void OrderedStoredValue::setDeletedTime(rel_time_t time) {
     if (!isDeleted()) {
         throw std::logic_error(
index 3c30fa9..86ee89c 100644 (file)
@@ -686,6 +686,11 @@ protected:
      */
     void deleteImpl(HashTable& ht);
 
+    /* Update the value for this SV from the given item.
+     * Implementation for StoredValue instances (dispatched to by setValue()).
+     */
+    void setValueImpl(const Item& itm, HashTable& ht);
+
     friend class HashTable;
     friend class StoredValueFactory;
     friend std::ostream& operator<<(std::ostream& os, const HashTable& ht);
@@ -794,6 +799,12 @@ protected:
      */
     void deleteImpl(HashTable& ht);
 
+    /* Update the value for this OSV from the given item.
+     * Implementation for OrderedStoredValue instances (dispatched to by
+     *  setValue()).
+     */
+    void setValueImpl(const Item& itm, HashTable& ht);
+
     /**
      * Set the time the item was deleted to the specified time.
      */
index ef50ec0..fd39c54 100644 (file)
@@ -425,3 +425,32 @@ TEST_F(EphTombstoneTest, ConcurrentPurge) {
     fe1.join();
     fe2.join();
 }
+
+// Test that on a double-delete (delete with a different value) the deleted time
+// is updated correctly.
+TEST_F(EphTombstoneTest, DoubleDeleteTimeCorrect) {
+    // Delete the first item at +0s
+    auto key = keys.at(0);
+    softDeleteOne(key, MutationStatus::WasDirty);
+    auto* delOSV = findValue(key)->toOrderedStoredValue();
+    auto initialDelTime = delOSV->getDeletedTime();
+    ASSERT_EQ(2, delOSV->getRevSeqno()) << "Should be initial set + 1";
+
+    // Advance to non-zero time.
+    const int timeJump = 10;
+    TimeTraveller nonZero(timeJump);
+    ASSERT_GE(ep_current_time(), initialDelTime + timeJump)
+            << "Failed to advance at least " + std::to_string(timeJump) +
+                       " seconds from when initial delete "
+                       "occcured";
+
+    // Delete the same key again (delete-with-Value), checking the deleted
+    // time has changed.
+    Item item(key, 0, 0, "deleted", strlen("deleted"));
+    item.setDeleted();
+    ASSERT_EQ(MutationStatus::WasDirty, public_processSet(item, item.getCas()));
+    ASSERT_EQ(3, delOSV->getRevSeqno()) << "Should be initial set + 2";
+
+    auto secondDelTime = delOSV->getDeletedTime();
+    EXPECT_GE(secondDelTime, initialDelTime + timeJump);
+}