MB-32734 [SR]: Add SyncDelete support to HashTable

Update HashTable::unlocked_softDelete() to take a SyncDelete argument which specifies if the delete should be performed durably. Similar to SyncWrites, this will stage an additional pending Item in the HashTable with the proposed deleted state. Upon commit() this will replace the existing item. Expand HashTablePerspectiveTest and VBucketDurabilityTest to cover the basic use-cases of this. Change-Id: Iab4bb4c384bce6e7680cb523de96ed8cd71cdc00 Reviewed-on: http://review.couchbase.org/104152 Tested-by: Build Bot <[email protected]> Reviewed-by: Paolo Cocchi <[email protected]>
couchbase · Feb 1, 2019 · f057e5a · f057e5a
1 parent 620f79b
commit f057e5a
Show file tree

Hide file tree

Showing 13 changed files with 280 additions and 62 deletions.
diff --git a/engines/ep/src/ep_vb.cc b/engines/ep/src/ep_vb.cc
@@ -562,7 +562,7 @@ std::tuple<StoredValue*, VBNotifyCtx> EPVBucket::softDeleteStoredValue(
         const VBQueueItemCtx& queueItmCtx,
         uint64_t bySeqno,
         DeleteSource deleteSource) {
-    ht.unlocked_softDelete(hbl.getHTLock(), v, onlyMarkDeleted, deleteSource);
+    ht.unlocked_softDelete(hbl, v, onlyMarkDeleted, deleteSource);
 
     if (queueItmCtx.genBySeqno == GenerateBySeqno::No) {
         v.setBySeqno(bySeqno);

diff --git a/engines/ep/src/ephemeral_vb.cc b/engines/ep/src/ephemeral_vb.cc
@@ -542,8 +542,7 @@ std::tuple<StoredValue*, VBNotifyCtx> EphemeralVBucket::softDeleteStoredValue(
         }
 
         /* Delete the storedvalue */
-        ht.unlocked_softDelete(
-                hbl.getHTLock(), *newSv, onlyMarkDeleted, deleteSource);
+        ht.unlocked_softDelete(hbl, *newSv, onlyMarkDeleted, deleteSource);
 
         if (queueItmCtx.genBySeqno == GenerateBySeqno::No) {
             newSv->setBySeqno(bySeqno);

diff --git a/engines/ep/src/hash_table.cc b/engines/ep/src/hash_table.cc
@@ -21,6 +21,7 @@
 #include "stats.h"
 #include "stored_value_factories.h"
 
+#include <memcached/3rd_party/folly/lang/Assume.h>
 #include <phosphor/phosphor.h>
 #include <platform/compress.h>
 
@@ -323,7 +324,7 @@ void HashTable::commit(const HashTable::HashBucketLock& hbl, StoredValue& v) {
     }
 
     // Change the pending item to Committed.
-    v.setCommitted();
+    v.setCommitted(CommittedState::CommittedViaPrepare);
 
     // Update stats for pending -> Committed item
     valueStats.epilogue(pendingPreProps, &v);
@@ -372,9 +373,7 @@ HashTable::UpdateResult HashTable::unlocked_updateStoredValue(
 
         return {status, &v};
     }
-
-    throw std::logic_error(
-            "HashTable::unlocked_updateStoredValue: unreachable");
+    folly::assume_unreachable();
 }
 
 StoredValue* HashTable::unlocked_addNewStoredValue(const HashBucketLock& hbl,
@@ -529,19 +528,54 @@ HashTable::unlocked_replaceByCopy(const HashBucketLock& hbl,
     return {values[hbl.getBucketNum()].get().get(), std::move(releasedSv)};
 }
 
-void HashTable::unlocked_softDelete(const std::unique_lock<std::mutex>& htLock,
-                                    StoredValue& v,
-                                    bool onlyMarkDeleted,
-                                    DeleteSource delSource) {
-    const auto preProps = valueStats.prologue(&v);
+HashTable::DeleteResult HashTable::unlocked_softDelete(
+        const HashBucketLock& hbl,
+        StoredValue& v,
+        bool onlyMarkDeleted,
+        DeleteSource delSource,
+        HashTable::SyncDelete syncDelete) {
+    switch (v.getCommitted()) {
+    case CommittedState::Pending:
+        // Cannot update a SV if it's a Pending item.
+        return {DeletionStatus::IsPendingSyncWrite, nullptr};
 
-    if (onlyMarkDeleted) {
-        v.markDeleted(delSource);
-    } else {
-        v.del(delSource);
-    }
+    case CommittedState::CommittedViaMutation:
+    case CommittedState::CommittedViaPrepare:
+        if (syncDelete == SyncDelete::Yes) {
+            // Logically we /can/ delete a non-Pending StoredValue with a
+            // SyncDelete, however internally this is implemented as a separate
+            // (new) StoredValue object for the Pending delete.
+
+            // Clone the existing StoredValue (for it's key, CAS, other
+            // metadata), set the opcode to pending and call del() to prepare it
+            // as a deleted item. The existing StoredValue keeps the same state
+            // until we Commit the pending one.
+            auto pendingDel = valFact->copyStoredValue(
+                    v, std::move(values[hbl.getBucketNum()]));
+            pendingDel->setCommitted(CommittedState::Pending);
+            pendingDel->del(delSource);
+
+            // Adding a new item into the HashTable; update stats.
+            const auto emptyProperties = valueStats.prologue(nullptr);
+            valueStats.epilogue(emptyProperties, pendingDel.get().get());
+            values[hbl.getBucketNum()] = std::move(pendingDel);
+
+            return {DeletionStatus::Success,
+                    values[hbl.getBucketNum()].get().get()};
+        }
+        // non-syncDelete requests, can directly update existing SV.
+        const auto preProps = valueStats.prologue(&v);
 
-    valueStats.epilogue(preProps, &v);
+        if (onlyMarkDeleted) {
+            v.markDeleted(delSource);
+        } else {
+            v.del(delSource);
+        }
+
+        valueStats.epilogue(preProps, &v);
+        return {DeletionStatus::Success, &v};
+    }
+    folly::assume_unreachable();
 }
 
 StoredValue* HashTable::unlocked_find(const DocKey& key,

diff --git a/engines/ep/src/hash_table.h b/engines/ep/src/hash_table.h
@@ -67,6 +67,12 @@ enum class TempAddStatus : uint8_t {
     BgFetch //Schedule a background fetch
 };
 
+/// Result from delete options.
+enum class DeletionStatus : uint8_t {
+    Success, // Item was successfully marked as deleted.
+    IsPendingSyncWrite, //!< The item a pending SyncWrite and can't be deleted.
+};
+
 /**
  * A container of StoredValue instances.
  *
@@ -747,21 +753,44 @@ class HashTable {
      */
     std::pair<StoredValue*, StoredValue::UniquePtr> unlocked_replaceByCopy(
             const HashBucketLock& hbl, const StoredValue& vToCopy);
+
+    enum class SyncDelete {
+        // A normal, non-synchronous, instant delete.
+        No,
+        /// A SyncDelete which not complete until it's durability requirements
+        // have been met.
+        Yes,
+    };
+
+    /**
+     * Result of a Delete operation.
+     */
+    struct DeleteResult {
+        /// Status of the operation.
+        DeletionStatus status;
+        // If the update was successful (Success); points to the delete value;
+        // otherwise nullptr.
+        StoredValue* deletedValue;
+    };
+
     /**
      * Logically (soft) delete the item in ht
      * Assumes that HT bucket lock is grabbed.
      * Also assumes that v is in the hash table.
      *
-     * @param htLock Hash table lock that must be held
+     * @param hbl Hash table bucket lock that must be held.
      * @param v Reference to the StoredValue to be soft deleted
      * @param onlyMarkDeleted indicates if we must reset the StoredValue or
      *                        just mark deleted
      * @param delSource The source of the deletion (explicit or expiry)
+     * @param syncDelete Is this a regular or durable delete?
+     * @return the outcome of the deletion attempt.
      */
-    void unlocked_softDelete(const std::unique_lock<std::mutex>& htLock,
-                             StoredValue& v,
-                             bool onlyMarkDeleted,
-                             DeleteSource delSource);
+    DeleteResult unlocked_softDelete(const HashBucketLock& hbl,
+                                     StoredValue& v,
+                                     bool onlyMarkDeleted,
+                                     DeleteSource delSource,
+                                     SyncDelete syncDelete = SyncDelete::No);
 
     /**
      * Find an item within a specific bucket assuming you already

diff --git a/engines/ep/src/stored-value.cc b/engines/ep/src/stored-value.cc
@@ -269,6 +269,10 @@ bool StoredValue::operator==(const StoredValue& other) const {
             getCommitted() == other.getCommitted());
 }
 
+bool StoredValue::operator!=(const StoredValue& other) const {
+    return !(*this == other);
+}
+
 bool StoredValue::deleteImpl(DeleteSource delSource) {
     if (isDeleted() && !getValue()) {
         // SV is already marked as deleted and has no value - no further

diff --git a/engines/ep/src/stored-value.h b/engines/ep/src/stored-value.h
@@ -734,6 +734,8 @@ class StoredValue {
      */
     bool operator==(const StoredValue& other) const;
 
+    bool operator!=(const StoredValue& other) const;
+
     /// Return how many bytes are need to store item given key as a StoredValue
     static size_t getRequiredStorage(const DocKey& key);
 
@@ -754,9 +756,9 @@ class StoredValue {
         return static_cast<CommittedState>(committed);
     }
 
-    /// Sets the Committed state of the SV to "Committed"
-    void setCommitted() {
-        committed = static_cast<uint8_t>(CommittedState::CommittedViaPrepare);
+    /// Sets the Committed state of the SV to the specified value.
+    void setCommitted(CommittedState value) {
+        committed = static_cast<uint8_t>(value);
     }
 
 protected:
@@ -868,11 +870,6 @@ class StoredValue {
         deletionSource = static_cast<uint8_t>(delSource);
     }
 
-    /// Sets the commited state to the specified value.
-    void setCommitted(CommittedState value) {
-        committed = static_cast<uint8_t>(value);
-    }
-
     friend class StoredValueFactory;
 
     /**

diff --git a/engines/ep/src/stored_value_factories.cc b/engines/ep/src/stored_value_factories.cc
@@ -31,6 +31,15 @@ StoredValue::UniquePtr StoredValueFactory::operator()(
                                 /*isOrdered*/ false));
 }
 
+StoredValue::UniquePtr StoredValueFactory::copyStoredValue(
+        const StoredValue& other, StoredValue::UniquePtr next) {
+    // Allocate a buffer to store the copy of StoredValue and any
+    // trailing bytes required for the key.
+    return StoredValue::UniquePtr(
+            new (::operator new(other.getObjectSize()))
+                    StoredValue(other, std::move(next), *stats));
+}
+
 StoredValue::UniquePtr OrderedStoredValueFactory::operator()(
         const Item& itm, StoredValue::UniquePtr next) {
     // Allocate a buffer to store the OrderStoredValue and any trailing

diff --git a/engines/ep/src/stored_value_factories.h b/engines/ep/src/stored_value_factories.h
@@ -72,10 +72,8 @@ class StoredValueFactory : public AbstractStoredValueFactory {
     StoredValue::UniquePtr operator()(const Item& itm,
                                       StoredValue::UniquePtr next) override;
 
-    StoredValue::UniquePtr copyStoredValue(const StoredValue& other,
-                                           StoredValue::UniquePtr next) override {
-        throw std::logic_error("Copy of StoredValue is not supported");
-    }
+    StoredValue::UniquePtr copyStoredValue(
+            const StoredValue& other, StoredValue::UniquePtr next) override;
 
 private:
     EPStats* stats;

diff --git a/engines/ep/tests/module_tests/basic_ll_test.cc b/engines/ep/tests/module_tests/basic_ll_test.cc
@@ -217,7 +217,7 @@ class BasicLinkedListTest : public ::testing::Test {
         { /* hbl lock scope */
             auto result = ht.findForWrite(makeStoredDocKey(key));
 
-            ht.unlocked_softDelete(result.lock.getHTLock(),
+            ht.unlocked_softDelete(result.lock,
                                    *result.storedValue,
                                    /* onlyMarkDeleted */ false,
                                    DeleteSource::Explicit);