MB-33332: Rename OSV::deletedTime to OSV::completedOrDeletedTime

To purge completed prepares only after the metadata purge interval
we need to know when the prepare was completed (committed or aborted).
We do similar for deleted items, we set a deleted time on deletion
of the OSV and use this time in the HTTombstonePurger to determine if
we should purge the metadata (tombstone) for the delete. We can simply
re-use the field for completed time. Rename the functions to something
more appropriate. They will be used in a later change.

Change-Id: Ia37b3a6bba9f3945280ed9c1dc2568998cc0e122
Reviewed-on: http://review.couchbase.org/109839
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: BenHuddleston

engines/ep/src/dcp/backfill_memory.cc

@@ -306,7 +306,8 @@ backfill_status_t DCPBackfillMemoryBuffered::scan() {
             // time field, this must be copied to the expiry time so that DCP
             // can transmit the original time of deletion
             if (item->isDeleted()) {
-                item->setExpTime(ep_abs_time((*rangeItr).getDeletedTime()));
+                item->setExpTime(
+                        ep_abs_time((*rangeItr).getCompletedOrDeletedTime()));
             }
         } catch (const std::bad_alloc&) {
             stream->log(spdlog::level::level_enum::warn,

engines/ep/src/ephemeral_tombstone_purger.cc

@@ -48,9 +48,8 @@ bool EphemeralVBucket::HTTombstonePurger::visit(
     // that we do not get a -ve value when we check if the time difference
     // is >= purgeAge. This is preferable to updating the task start time for
     // every visit and has little impact as this task runs frequently.
-    if (osv->isDeleted() &&
-        (now >= osv->getDeletedTime()) &&
-        (now - osv->getDeletedTime() >= purgeAge)) {
+    if (osv->isDeleted() && (now >= osv->getCompletedOrDeletedTime()) &&
+        (now - osv->getCompletedOrDeletedTime() >= purgeAge)) {
         // This item should be purged. Remove from the HashTable and move over
         // to being owned by the sequence list.
         auto ownedSV = vbucket->ht.unlocked_release(hbl, v.getKey());

engines/ep/src/ephemeral_vb.cc

@@ -618,7 +618,7 @@ EphemeralVBucket::softDeleteStoredValue(const HashTable::HashBucketLock& hbl,
             // The deleted time is relative and the replicated tombstone time is
             // absolute and held in the expiry field, convert the abs to rel
             // using ep_reltime
-            newSv->toOrderedStoredValue()->setDeletedTime(
+            newSv->toOrderedStoredValue()->setCompletedOrDeletedTime(
                     ep_reltime(newSv->getExptime()));
         }
 

engines/ep/src/stored-value.cc

@@ -38,7 +38,7 @@ StoredValue::StoredValue(const Item& itm,
       chain_next_or_replacement(std::move(n)),
       cas(itm.getCas()),
       bySeqno(itm.getBySeqno()),
-      lock_expiry_or_delete_time(0),
+      lock_expiry_or_delete_or_complete_time(0),
       exptime(itm.getExptime()),
       flags(itm.getFlags()),
       revSeqno(itm.getRevSeqno()),
@@ -86,7 +86,8 @@ StoredValue::StoredValue(const StoredValue& other, UniquePtr n, EPStats& stats)
       chain_next_or_replacement(std::move(n)),
       cas(other.cas),
       bySeqno(other.bySeqno),
-      lock_expiry_or_delete_time(other.lock_expiry_or_delete_time),
+      lock_expiry_or_delete_or_complete_time(
+              other.lock_expiry_or_delete_or_complete_time),
       exptime(other.exptime),
       flags(other.flags),
       revSeqno(other.revSeqno),
@@ -292,7 +293,8 @@ const OrderedStoredValue* StoredValue::toOrderedStoredValue() const {
 bool StoredValue::operator==(const StoredValue& other) const {
     return (cas == other.cas && revSeqno == other.revSeqno &&
             bySeqno == other.bySeqno &&
-            lock_expiry_or_delete_time == other.lock_expiry_or_delete_time &&
+            lock_expiry_or_delete_or_complete_time ==
+                    other.lock_expiry_or_delete_or_complete_time &&
             exptime == other.exptime && flags == other.flags &&
             isDirty() == other.isDirty() && isDeleted() == other.isDeleted() &&
             // Note: deletionCause is only checked if the item is deleted
@@ -368,7 +370,7 @@ void StoredValue::setValueImpl(const Item& itm) {
     datatype = itm.getDataType();
     bySeqno = itm.getBySeqno();
     cas = itm.getCas();
-    lock_expiry_or_delete_time = 0;
+    lock_expiry_or_delete_or_complete_time = 0;
     exptime = itm.getExptime();
     revSeqno = itm.getRevSeqno();
 
@@ -518,7 +520,7 @@ std::ostream& operator<<(std::ostream& os, const StoredValue& sv) {
     os << " cas:" << sv.getCas();
     os << " key:\"" << sv.getKey() << "\"";
     if (sv.isOrdered() && sv.isDeleted()) {
-        os << " del_time:" << sv.lock_expiry_or_delete_time;
+        os << " del_time:" << sv.lock_expiry_or_delete_or_complete_time;
     } else {
         os << " exp:" << sv.getExptime();
     }
@@ -552,11 +554,12 @@ size_t OrderedStoredValue::getRequiredStorage(const DocKey& key) {
 }
 
 /**
- * Return the time the item was deleted. Only valid for deleted items.
+ * Return the time the item was deleted. Only valid for completed or deleted
+ * items.
  */
-rel_time_t OrderedStoredValue::getDeletedTime() const {
-    if (isDeleted()) {
-        return lock_expiry_or_delete_time;
+rel_time_t OrderedStoredValue::getCompletedOrDeletedTime() const {
+    if (isDeleted() || isPending()) {
+        return lock_expiry_or_delete_or_complete_time;
     } else {
         throw std::logic_error(
                 "OrderedStoredValue::getDeletedItem: Called on Alive item");
@@ -567,7 +570,7 @@ bool OrderedStoredValue::deleteImpl(DeleteSource delSource) {
     if (StoredValue::deleteImpl(delSource)) {
         // Need to record the time when an item is deleted for subsequent
         //purging (ephemeral_metadata_purge_age).
-        setDeletedTime(ep_current_time());
+        setCompletedOrDeletedTime(ep_current_time());
         return true;
     }
     return false;
@@ -579,14 +582,15 @@ void OrderedStoredValue::setValueImpl(const Item& itm) {
     // Update the deleted time (note - even if it was already deleted we should
     // refresh this).
     if (isDeleted()) {
-        setDeletedTime(ep_current_time());
+        setCompletedOrDeletedTime(ep_current_time());
     }
 }
 
-void OrderedStoredValue::setDeletedTime(rel_time_t time) {
-    if (!isDeleted()) {
+void OrderedStoredValue::setCompletedOrDeletedTime(rel_time_t time) {
+    if (!isDeleted() && !isPending()) {
         throw std::logic_error(
-                "OrderedStoredValue::setDeletedTime: Called on Alive item");
+                "OrderedStoredValue::setCompletedOrDeletedTime: Called on "
+                "Alive item");
     }
-    lock_expiry_or_delete_time = time;
+    lock_expiry_or_delete_or_complete_time = time;
 }

engines/ep/src/stored-value.h

@@ -398,7 +398,7 @@ class StoredValue {
             throw std::logic_error(
                     "StoredValue::lock: Called on Deleted item");
         }
-        lock_expiry_or_delete_time = expiry;
+        lock_expiry_or_delete_or_complete_time = expiry;
     }
 
     /**
@@ -409,7 +409,7 @@ class StoredValue {
             // Deleted items are not locked - just skip.
             return;
         }
-        lock_expiry_or_delete_time = 0;
+        lock_expiry_or_delete_or_complete_time = 0;
     }
 
     /**
@@ -545,8 +545,8 @@ class StoredValue {
             return false;
         }
 
-        if (lock_expiry_or_delete_time == 0 ||
-            (curtime > lock_expiry_or_delete_time)) {
+        if (lock_expiry_or_delete_or_complete_time == 0 ||
+            (curtime > lock_expiry_or_delete_or_complete_time)) {
             return false;
         }
         return true;
@@ -997,8 +997,9 @@ class StoredValue {
     // this in ephemeral backfills with different locks (which is true, but the
     // access is we believe actually safe)
     cb::RelaxedAtomic<int64_t> bySeqno; //!< By sequence id number
-    /// For alive items: GETL lock expiration. For deleted items: delete time.
-    rel_time_t         lock_expiry_or_delete_time;
+    // For alive items: GETL lock expiration. For deleted items: delete time.
+    // For prepared items: the time at which they were completed.
+    rel_time_t lock_expiry_or_delete_or_complete_time;
     uint32_t           exptime;        //!< Expiration time of this item.
     uint32_t           flags;          // 4 bytes
     cb::uint48_t revSeqno; //!< Revision id sequence number
@@ -1095,9 +1096,10 @@ class OrderedStoredValue : public StoredValue {
     }
 
     /**
-     * Return the time the item was deleted. Only valid for deleted items.
+     * Return the time the item was deleted. Only valid for completed
+     * (SyncWrites) or deleted items.
      */
-    rel_time_t getDeletedTime() const;
+    rel_time_t getCompletedOrDeletedTime() const;
 
     /**
      * Check if the contents of the StoredValue is same as that of the other
@@ -1123,9 +1125,10 @@ class OrderedStoredValue : public StoredValue {
     }
 
     /**
-     * Set the time the item was deleted to the specified time.
+     * Set the time the item was completed (SyncWrite) or deleted at to the
+     * specified time.
      */
-    void setDeletedTime(rel_time_t time);
+    void setCompletedOrDeletedTime(rel_time_t time);
 
 protected:
     SerialisedDocKey* key() {

engines/ep/tests/module_tests/ephemeral_vb_test.cc

@@ -762,7 +762,7 @@ TEST_F(EphTombstoneTest, DoubleDeleteTimeCorrect) {
     auto key = keys.at(0);
     softDeleteOne(key, MutationStatus::WasDirty);
     auto* delOSV = findValue(key)->toOrderedStoredValue();
-    auto initialDelTime = delOSV->getDeletedTime();
+    auto initialDelTime = delOSV->getCompletedOrDeletedTime();
     ASSERT_EQ(2, delOSV->getRevSeqno()) << "Should be initial set + 1";
 
     // Advance to non-zero time.
@@ -780,7 +780,7 @@ TEST_F(EphTombstoneTest, DoubleDeleteTimeCorrect) {
     ASSERT_EQ(MutationStatus::WasDirty, public_processSet(item, item.getCas()));
     ASSERT_EQ(3, delOSV->getRevSeqno()) << "Should be initial set + 2";
 
-    auto secondDelTime = delOSV->getDeletedTime();
+    auto secondDelTime = delOSV->getCompletedOrDeletedTime();
     EXPECT_GE(secondDelTime, initialDelTime + timeJump);
 }
 

engines/ep/tests/module_tests/evp_store_single_threaded_test.cc

@@ -1898,7 +1898,8 @@ TEST_P(STParameterizedBucketTest, MB_27457) {
         auto* sv = ro.storedValue;
         ASSERT_NE(nullptr, sv);
         deleted = sv->isDeleted();
-        tombstoneTime = uint64_t(sv->toOrderedStoredValue()->getDeletedTime());
+        tombstoneTime = uint64_t(
+                sv->toOrderedStoredValue()->getCompletedOrDeletedTime());
     }
 
     EXPECT_EQ(1, deleted);
@@ -1935,8 +1936,8 @@ TEST_P(STParameterizedBucketTest, MB_27457) {
         auto* sv = ro.storedValue;
         ASSERT_NE(nullptr, sv);
         deleted = sv->isDeleted();
-        tombstoneTime =
-                ep_abs_time((sv->toOrderedStoredValue()->getDeletedTime()));
+        tombstoneTime = ep_abs_time(
+                (sv->toOrderedStoredValue()->getCompletedOrDeletedTime()));
     }
     EXPECT_EQ(1, deleted);
     EXPECT_EQ(PROTOCOL_BINARY_RAW_BYTES, datatype);

engines/ep/tests/module_tests/hash_table_test.cc

@@ -1040,7 +1040,7 @@ TEST_F(HashTableTest, LockAfterDelete) {
 
     // Sanity check - deleted time should be set.
     auto* osv = sv->toOrderedStoredValue();
-    ASSERT_GE(osv->getDeletedTime(), 1985);
+    ASSERT_GE(osv->getCompletedOrDeletedTime(), 1985);
 
     // Now re-create the same key (as alive).
     Item i(key, 0, 0, key.data(), key.size());