MB-35631: Read vbstate from disk in initScanContext

We should not read the vbucket state from cache when creating a
ScanContext as the cached value may be ahead.

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

Author: BenHuddleston

engines/ep/src/couch-kvstore/couch-kvstore.cc

@@ -382,7 +382,7 @@ void CouchKVStore::initialize() {
         DbHolder db(*this);
         errorCode = openDB(id, db, COUCHSTORE_OPEN_FLAG_RDONLY);
         if (errorCode == COUCHSTORE_SUCCESS) {
-            auto readStatus = readVBState(db, id);
+            auto readStatus = readVBStateAndUpdateCache(db, id).status;
             if (readStatus == ReadVBStateStatus::Success) {
                 /* update stat */
                 ++st.numLoadedVb;
@@ -1457,17 +1457,12 @@ ScanContext* CouchKVStore::initScanContext(
         return NULL;
     }
 
-    auto* vbState = getVBucketState(vbid);
-    if (!vbState) {
-        if (readVBState(db, vbid) == CouchKVStore::ReadVBStateStatus::Success) {
-            vbState = getVBucketState(vbid);
-        }
-        if (!vbState) {
-            EP_LOG_WARN(
-                    "CouchKVStore::initScanContext:Failed to obtain vbState for"
-                    "the highCompletedSeqno");
-            return NULL;
-        }
+    auto readVbStateResult = readVBState(db, vbid);
+    if (readVbStateResult.status != ReadVBStateStatus::Success) {
+        EP_LOG_WARN(
+                "CouchKVStore::initScanContext:Failed to obtain vbState for"
+                "the highCompletedSeqno");
+        return NULL;
     }
 
     size_t scanId = scanCounter++;
@@ -1479,19 +1474,20 @@ ScanContext* CouchKVStore::initScanContext(
         scans[scanId] = db.releaseDb();
     }
 
-    ScanContext* sctx = new ScanContext(cb,
-                                        cl,
-                                        vbid,
-                                        scanId,
-                                        startSeqno,
-                                        info.last_sequence,
-                                        info.purge_seq,
-                                        options,
-                                        valOptions,
-                                        count,
-                                        vbState->highCompletedSeqno,
-                                        configuration,
-                                        collectionsManifest);
+    ScanContext* sctx =
+            new ScanContext(cb,
+                            cl,
+                            vbid,
+                            scanId,
+                            startSeqno,
+                            info.last_sequence,
+                            info.purge_seq,
+                            options,
+                            valOptions,
+                            count,
+                            readVbStateResult.state.highCompletedSeqno,
+                            configuration,
+                            collectionsManifest);
     sctx->logger = &logger;
     return sctx;
 }
@@ -2358,7 +2354,7 @@ CouchKVStore::processVbstateSnapshot(Vbid vb,
     return {status, snapStart, snapEnd};
 }
 
-CouchKVStore::ReadVBStateStatus CouchKVStore::readVBState(Db* db, Vbid vbId) {
+CouchKVStore::ReadVBStateResult CouchKVStore::readVBState(Db* db, Vbid vbId) {
     sized_buf id;
     LocalDoc *ldoc = NULL;
     ReadVBStateStatus status = ReadVBStateStatus::Success;
@@ -2378,7 +2374,7 @@ CouchKVStore::ReadVBStateStatus CouchKVStore::readVBState(Db* db, Vbid vbId) {
                 ", {}",
                 couchstore_strerror(couchStoreStatus),
                 vbId);
-        return ReadVBStateStatus::CouchstoreError;
+        return {ReadVBStateStatus::CouchstoreError, {}};
     }
 
     vbucket_state vbState;
@@ -2399,7 +2395,7 @@ CouchKVStore::ReadVBStateStatus CouchKVStore::readVBState(Db* db, Vbid vbId) {
                 " error:{}, {}",
                 couchstore_strerror(couchStoreStatus),
                 vbId);
-        return ReadVBStateStatus::CouchstoreError;
+        return {ReadVBStateStatus::CouchstoreError, {}};
     }
 
     // Proceed to read/parse the vbstate if success
@@ -2418,7 +2414,7 @@ CouchKVStore::ReadVBStateStatus CouchKVStore::readVBState(Db* db, Vbid vbId) {
                     vbId,
                     statjson,
                     e.what());
-            return ReadVBStateStatus::JsonInvalid;
+            return {ReadVBStateStatus::JsonInvalid, {}};
         }
 
         // Merge in the high_seqno & purge_seqno read previously from db info.
@@ -2437,7 +2433,7 @@ CouchKVStore::ReadVBStateStatus CouchKVStore::readVBState(Db* db, Vbid vbId) {
                     vbId,
                     json.dump(),
                     e.what());
-            return ReadVBStateStatus::JsonInvalid;
+            return {ReadVBStateStatus::JsonInvalid, {}};
         }
 
         // MB-17517: If the maxCas on disk was invalid then don't use it -
@@ -2465,12 +2461,18 @@ CouchKVStore::ReadVBStateStatus CouchKVStore::readVBState(Db* db, Vbid vbId) {
         couchstore_free_local_document(ldoc);
     }
 
-    if (status == ReadVBStateStatus::Success) {
+    return {status, vbState};
+}
+
+CouchKVStore::ReadVBStateResult CouchKVStore::readVBStateAndUpdateCache(
+        Db* db, Vbid vbid) {
+    auto res = readVBState(db, vbid);
+    if (res.status == ReadVBStateStatus::Success) {
         // Cannot use make_unique here as it doesn't support
         // brace-initialization until C++20.
-        cachedVBStates[vbId.get()].reset(new vbucket_state(vbState));
+        cachedVBStates[vbid.get()].reset(new vbucket_state(res.state));
     }
-    return status;
+    return res;
 }
 
 couchstore_error_t CouchKVStore::saveVBState(Db *db,
@@ -2876,7 +2878,8 @@ RollbackResult CouchKVStore::rollback(Vbid vbid,
         return RollbackResult(false);
     }
 
-    if (readVBState(newdb, vbid) != ReadVBStateStatus::Success) {
+    if (readVBStateAndUpdateCache(newdb, vbid).status !=
+        ReadVBStateStatus::Success) {
         return RollbackResult(false);
     }
     cachedDeleteCount[vbid.get()] = info.deleted_count;

engines/ep/src/couch-kvstore/couch-kvstore.h

@@ -29,12 +29,12 @@
 #include <platform/strerror.h>
 #include <relaxed_atomic.h>
 
+#include <engines/ep/src/vbucket_state.h>
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
-
 #define COUCHSTORE_NO_OPTIONS 0
 
 class EventuallyPersistentEngine;
@@ -341,27 +341,6 @@ class CouchKVStore : public KVStore
     static int recordDbStat(Db *db, DocInfo *docinfo, void *ctx);
     static int getMultiCb(Db *db, DocInfo *docinfo, void *ctx);
 
-    enum class ReadVBStateStatus {
-        Success,
-        JsonInvalid,
-        CorruptSnapshot,
-        CouchstoreError
-    };
-
-    ReadVBStateStatus readVBState(Db* db, Vbid vbId);
-
-    /**
-     * Process the vbstate snapshot strings which are stored in the vbstate
-     * document. Check for validity and return a status + decoded snapshot.
-     */
-    std::tuple<ReadVBStateStatus, uint64_t, uint64_t> processVbstateSnapshot(
-            Vbid vb,
-            vbucket_state_t state,
-            int64_t version,
-            uint64_t snapStart,
-            uint64_t snapEnd,
-            uint64_t highSeqno);
-
     couchstore_error_t fetchDoc(Db* db,
                                 DocInfo* docinfo,
                                 GetValue& docValue,
@@ -757,6 +736,45 @@ class CouchKVStore : public KVStore
     /// Copy relevant DbInfo stats to the common FileStats struct
     static FileInfo toFileInfo(const DbInfo& info);
 
+    enum class ReadVBStateStatus {
+        Success,
+        JsonInvalid,
+        CorruptSnapshot,
+        CouchstoreError
+    };
+
+    /**
+     * Result of the readVBState function
+     */
+    struct ReadVBStateResult {
+        ReadVBStateStatus status;
+
+        // Only valid if status == ReadVBStateStatus::Success
+        vbucket_state state;
+    };
+
+    /**
+     * Process the vbstate snapshot strings which are stored in the vbstate
+     * document. Check for validity and return a status + decoded snapshot.
+     */
+    std::tuple<ReadVBStateStatus, uint64_t, uint64_t> processVbstateSnapshot(
+            Vbid vb,
+            vbucket_state_t state,
+            int64_t version,
+            uint64_t snapStart,
+            uint64_t snapEnd,
+            uint64_t highSeqno);
+
+    /**
+     * Read the vbucket_state from disk.
+     */
+    ReadVBStateResult readVBState(Db* db, Vbid vbid);
+
+    /**
+     * Read the vbucket_state from disk and update the cache if successful
+     */
+    ReadVBStateResult readVBStateAndUpdateCache(Db* db, Vbid vbid);
+
     const std::string dbname;
 
     using MonotonicRevision = AtomicMonotonic<uint64_t, ThrowExceptionPolicy>;

engines/ep/tests/module_tests/kvstore_test.cc

@@ -1137,8 +1137,13 @@ TEST_F(CouchKVStoreErrorInjectionTest, readVBState_open_local_document) {
                 .RetiresOnSaturation();
 
         /* Establish FileOps expectation */
+        // Called twice, once when we read the vbstate from disk in
+        // initScanContext, and again when we read the vbstate as part of
+        // rollback.
         EXPECT_CALL(ops, pread(_, _, _, _, _))
-            .WillOnce(Return(COUCHSTORE_ERROR_READ)).RetiresOnSaturation();
+                .Times(2)
+                .WillRepeatedly(Return(COUCHSTORE_ERROR_READ))
+                .RetiresOnSaturation();
         EXPECT_CALL(ops, pread(_, _, _, _, _)).Times(20).RetiresOnSaturation();
 
         kvstore->rollback(Vbid(0), 5, rcb);