MB-38444: Remove HighestDedupSeqno/EarlySnapEndSeqno

They were used only for computing the range of a consistent snapshot at
Backfill. Now we always set (EndSeqno = HighSeqno) at Backfill, which is
a consistent range by definition.

Change-Id: I962414a201fcc06d6d58122cfe6189cf98b95624
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/144163
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>
Reviewed-by: James Harrison <[email protected]>
Well-Formed: Build Bot <[email protected]>

Author: paolococchi

engines/ep/src/ephemeral_vb.cc

@@ -193,10 +193,6 @@ void EphemeralVBucket::addStats(VBucketStatsDetailLevel detail,
                 add_stat,
                 c);
         addStat("seqlist_high_seqno", seqList->getHighSeqno(), add_stat, c);
-        addStat("seqlist_highest_deduped_seqno",
-                seqList->getHighestDedupedSeqno(),
-                add_stat,
-                c);
         addStat("seqlist_purged_count", seqListPurgeCount.load(), add_stat, c);
 
         uint64_t rr_begin, rr_end;
@@ -395,17 +391,15 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
 
     std::lock_guard<std::mutex> lh(sequenceLock);
 
-    const bool wasTemp = v.isTempItem();
     const bool oldValueDeleted = v.isDeleted();
     const bool recreatingDeletedItem = v.isDeleted() && !itm.isDeleted();
 
     StoredValue::UniquePtr ownedSv;
 
     {
-        // Once we update the seqList, there is a short period where the
-        // highSeqno and highestDedupedSeqno are both incorrect. We have to hold
-        // this lock to prevent a new rangeRead starting, and covering an
-        // inconsistent range.
+        // Once we update the seqList, there is a short period where highSeqno
+        // is incorrect. We have to hold this lock to prevent a new rangeRead
+        // starting, and covering an inconsistent range.
         std::lock_guard<std::mutex> listWriteLg(seqList->getListWriteLock());
 
         /* Update in the Ordered data structure (seqList) first and then update
@@ -451,12 +445,6 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
 
         seqList->maybeUpdateMaxVisibleSeqno(lh, listWriteLg, osv);
 
-        /* Temp items are never added to the seqList, hence updating a temp
-           item should not update the deduped seqno */
-        if (!wasTemp) {
-            seqList->updateHighestDedupedSeqno(listWriteLg, osv);
-        }
-
         if (res == SequenceList::UpdateStatus::Append) {
             /* Mark the un-updated storedValue as stale. This must be done after
                the new storedvalue for the item is visible for range read in the
@@ -555,10 +543,9 @@ EphemeralVBucket::softDeleteStoredValue(const HashTable::HashBucketLock& hbl,
 
     VBNotifyCtx notifyCtx;
     {
-        // Once we update the seqList, there is a short period where the
-        // highSeqno and highestDedupedSeqno are both incorrect. We have to hold
-        // this lock to prevent a new rangeRead starting, and covering an
-        // inconsistent range.
+        // Once we update the seqList, there is a short period where highSeqno
+        // is incorrect. We have to hold this lock to prevent a new rangeRead
+        // starting, and covering an inconsistent range.
         std::lock_guard<std::mutex> listWriteLg(seqList->getListWriteLock());
 
         /* Update the in the Ordered data structure (seqList) first and then
@@ -618,12 +605,6 @@ EphemeralVBucket::softDeleteStoredValue(const HashTable::HashBucketLock& hbl,
 
         seqList->maybeUpdateMaxVisibleSeqno(lh, listWriteLg, osv);
 
-        /* Temp items are never added to the seqList, hence updating a temp
-           item should not update the deduped seqno */
-        if (wasCommittedNonTemp) {
-            seqList->updateHighestDedupedSeqno(listWriteLg, osv);
-        }
-
         if (res == SequenceList::UpdateStatus::Append) {
             /* Mark the un-updated storedValue as stale. This must be done after
                the new storedvalue for the item is visible for range read in the
@@ -782,11 +763,6 @@ VBNotifyCtx EphemeralVBucket::abortStoredValue(
         if (res == SequenceList::UpdateStatus::Append) {
             seqList->markItemStale(listWriteLg, std::move(oldSv), newSv);
         }
-
-        // We de-duped a prepare so we need to update the highest deduped seqno
-        // to prevent a backfill range read from ending without reaching this
-        // abort
-        seqList->updateHighestDedupedSeqno(listWriteLg, osv);
     }
 
     return notifyCtx;

engines/ep/src/linked_list.cc

@@ -28,7 +28,6 @@ BasicLinkedList::BasicLinkedList(Vbid vbucketId, EPStats& st)
       staleSize(0),
       staleMetaDataSize(0),
       highSeqno(0),
-      highestDedupedSeqno(0),
       highestPurgedDeletedSeqno(0),
       numStaleItems(0),
       numDeletedItems(0),
@@ -100,19 +99,6 @@ void BasicLinkedList::updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
     highSeqno = v.getBySeqno();
 }
 
-void BasicLinkedList::updateHighestDedupedSeqno(
-        std::lock_guard<std::mutex>& listWriteLg, const OrderedStoredValue& v) {
-    if (v.getBySeqno() < 1) {
-        throw std::invalid_argument(
-                "BasicLinkedList::updateHighestDedupedSeqno(): " +
-                vbid.to_string() +
-                "; Cannot set the highestDedupedSeqno to "
-                "a value " +
-                std::to_string(v.getBySeqno()) + " which is < 1");
-    }
-    highestDedupedSeqno = v.getBySeqno();
-}
-
 void BasicLinkedList::maybeUpdateMaxVisibleSeqno(
         std::lock_guard<std::mutex>& seqLock,
         std::lock_guard<std::mutex>& writeLock,
@@ -337,11 +323,6 @@ uint64_t BasicLinkedList::getHighSeqno() const {
     return highSeqno;
 }
 
-uint64_t BasicLinkedList::getHighestDedupedSeqno() const {
-    std::lock_guard<std::mutex> lckGd(getListWriteLock());
-    return highestDedupedSeqno;
-}
-
 seqno_t BasicLinkedList::getHighestPurgedDeletedSeqno() const {
     return highestPurgedDeletedSeqno;
 }
@@ -438,7 +419,6 @@ BasicLinkedList::RangeIteratorLL::RangeIteratorLL(BasicLinkedList& ll,
     : list(ll),
       itrRange(0, 0),
       numRemaining(0),
-      earlySnapShotEndSeqno(0),
       maxVisibleSeqno(0),
       isBackfill(isBackfill) {
 
@@ -456,10 +436,6 @@ BasicLinkedList::RangeIteratorLL::RangeIteratorLL(BasicLinkedList& ll,
     /* Number of items that can be iterated over */
     numRemaining = list.seqList.size();
 
-    /* The minimum seqno in the iterator that must be read to get a consistent
-       read snapshot */
-    earlySnapShotEndSeqno = list.highestDedupedSeqno;
-
     maxVisibleSeqno = list.maxVisibleSeqno;
 
     /* Mark the snapshot range on linked list. The range that can be read by the
@@ -487,12 +463,10 @@ BasicLinkedList::RangeIteratorLL::RangeIteratorLL(BasicLinkedList& ll,
                                : spdlog::level::level_enum::debug;
 
     EP_LOG_FMT(severity,
-               "{} Created range iterator from {} to {}, earlySnapEndSeqno:{}, "
-               "maxVisibleSeqno:{}",
+               "{} Created range iterator from {} to {}, maxVisibleSeqno:{}",
                list.vbid,
                itrRange.getBegin(),
                itrRange.getEnd(),
-               earlySnapShotEndSeqno,
                maxVisibleSeqno);
 }
 

engines/ep/src/linked_list.h

@@ -99,9 +99,6 @@ class BasicLinkedList : public SequenceList {
     void updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
                          const OrderedStoredValue& v) override;
 
-    void updateHighestDedupedSeqno(std::lock_guard<std::mutex>& listWriteLg,
-                                   const OrderedStoredValue& v) override;
-
     void maybeUpdateMaxVisibleSeqno(std::lock_guard<std::mutex>& seqLock,
                                     std::lock_guard<std::mutex>& writeLock,
                                     const OrderedStoredValue& newSV) override;
@@ -130,8 +127,6 @@ class BasicLinkedList : public SequenceList {
 
     uint64_t getHighSeqno() const override;
 
-    uint64_t getHighestDedupedSeqno() const override;
-
     seqno_t getHighestPurgedDeletedSeqno() const override;
 
     uint64_t getMaxVisibleSeqno() const override;
@@ -233,14 +228,6 @@ class BasicLinkedList : public SequenceList {
      */
     Monotonic<seqno_t> highSeqno;
 
-    /**
-     * We need to this to send out point-in-time snapshots in range read
-     *
-     * highestDedupedSeqno is monotonically increasing and is reset to a lower
-     * value only in case of a rollback.
-     */
-    Monotonic<seqno_t> highestDedupedSeqno;
-
     /**
      * The sequence number of the highest purged element.
      *
@@ -327,10 +314,6 @@ class BasicLinkedList : public SequenceList {
             return numRemaining;
         }
 
-        seqno_t getEarlySnapShotEnd() const override {
-            return earlySnapShotEndSeqno;
-        }
-
         uint64_t getMaxVisibleSeqno() const override {
             return maxVisibleSeqno;
         }
@@ -385,10 +368,6 @@ class BasicLinkedList : public SequenceList {
            iterator at that instance */
         uint64_t numRemaining;
 
-        /* Indicates the minimum seqno in the iterator that can give a
-           consistent read snapshot */
-        seqno_t earlySnapShotEndSeqno;
-
         uint64_t maxVisibleSeqno;
 
         /* Indicates if the range iterator is for DCP backfill

engines/ep/src/seqlist.cc

@@ -52,10 +52,6 @@ uint64_t SequenceList::RangeIterator::count() const {
     return rangeIterImpl->count();
 }
 
-seqno_t SequenceList::RangeIterator::getEarlySnapShotEnd() const {
-    return rangeIterImpl->getEarlySnapShotEnd();
-}
-
 uint64_t SequenceList::RangeIterator::getMaxVisibleSeqno() const {
     return rangeIterImpl->getMaxVisibleSeqno();
 }

engines/ep/src/seqlist.h

@@ -118,12 +118,6 @@ class SequenceList {
          */
         virtual uint64_t count() const = 0;
 
-        /**
-         * Indicates the minimum seqno in the iterator that must be read to
-         * get a consistent read snapshot
-         */
-        virtual seqno_t getEarlySnapShotEnd() const = 0;
-
         virtual uint64_t getMaxVisibleSeqno() const = 0;
     };
 
@@ -216,12 +210,6 @@ class SequenceList {
          */
         uint64_t count() const;
 
-        /**
-         * Indicates the minimum seqno in the iterator that must be read to
-         * get a consistent read snapshot
-         */
-        seqno_t getEarlySnapShotEnd() const;
-
         uint64_t getMaxVisibleSeqno() const;
 
     private:
@@ -275,19 +263,6 @@ class SequenceList {
     virtual void updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
                                  const OrderedStoredValue& v) = 0;
 
-    /**
-     * Updates the highestDedupedSeqno in the list. Since seqno is generated and
-     * managed outside the list, the module managing it must update this after
-     * the seqno is generated for the item already put in the list.
-     *
-     * @param listWriteLg Write lock of the sequenceList from getListWriteLock()
-     * @param v Ref to orderedStoredValue
-     *
-     */
-    virtual void updateHighestDedupedSeqno(
-            std::lock_guard<std::mutex>& listWriteLg,
-            const OrderedStoredValue& v) = 0;
-
     /**
      * Updates the max-visible-seqno to the seqno of the new StoredValue, only
      * if the new item is committed.
@@ -400,11 +375,6 @@ class SequenceList {
      */
     virtual uint64_t getHighSeqno() const = 0;
 
-    /**
-     * Returns the highest de-duplicated sequence number in the list.
-     */
-    virtual uint64_t getHighestDedupedSeqno() const = 0;
-
     /**
      * Returns the highest purged Deleted sequence number in the list.
      */

engines/ep/tests/ep_testsuite.cc

@@ -7619,7 +7619,6 @@ static enum test_result test_mb19687_fixed(EngineIface* h) {
                            "vb_0:seqlist_count",
                            "vb_0:seqlist_deleted_count",
                            "vb_0:seqlist_high_seqno",
-                           "vb_0:seqlist_highest_deduped_seqno",
                            "vb_0:seqlist_purged_count",
                            "vb_0:seqlist_range_read_begin",
                            "vb_0:seqlist_range_read_count",

engines/ep/tests/module_tests/ephemeral_bucket_test.cc

@@ -60,7 +60,6 @@ TEST_F(EphemeralBucketStatTest, VBSeqlistStats) {
         << "Expected both current and deleted documents";
     EXPECT_EQ("1", stats.at("vb_0:seqlist_deleted_count"));
     EXPECT_EQ("4", stats.at("vb_0:seqlist_high_seqno"));
-    EXPECT_EQ("4", stats.at("vb_0:seqlist_highest_deduped_seqno"));
     EXPECT_EQ("0", stats.at("vb_0:seqlist_range_read_begin"));
     EXPECT_EQ("0", stats.at("vb_0:seqlist_range_read_end"));
     EXPECT_EQ("0", stats.at("vb_0:seqlist_range_read_count"));

engines/ep/tests/module_tests/ephemeral_vb_test.cc

@@ -781,40 +781,6 @@ TEST_F(EphTombstoneTest, DoubleDeleteTimeCorrect) {
     EXPECT_GE(secondDelTime, initialDelTime + timeJump);
 }
 
-TEST_F(EphemeralVBucketTest, UpdateUpdatesHighestDedupedSeqno) {
-    /* Add 3 items and then update all of them */
-    const int numItems = 3;
-
-    auto keys = generateKeys(numItems);
-    setMany(keys, MutationStatus::WasClean);
-
-    ASSERT_EQ(0, mockEpheVB->getLL()->getHighestDedupedSeqno());
-
-    /* Update the items */
-    setMany(keys, MutationStatus::WasDirty);
-
-    EXPECT_EQ(6, mockEpheVB->getLL()->getHighestDedupedSeqno());
-}
-
-TEST_F(EphemeralVBucketTest, AppendUpdatesHighestDedupedSeqno) {
-    /* Add 3 items and then update all of them */
-    const int numItems = 3;
-
-    auto keys = generateKeys(numItems);
-    setMany(keys, MutationStatus::WasClean);
-
-    ASSERT_EQ(0, mockEpheVB->getLL()->getHighestDedupedSeqno());
-
-    {
-        auto itr = mockEpheVB->getLL()->makeRangeIterator(true /*isBackfill*/);
-
-        /* Update the items */
-        setMany(keys, MutationStatus::WasClean);
-    }
-
-    ASSERT_EQ(6, mockEpheVB->getLL()->getHighestDedupedSeqno());
-}
-
 // Check that tombstone purger runs fine in pause-resume mode
 TEST_F(EphTombstoneTest, PurgePauseResume) {
     // Delete the second item