MB-43621: Don't count aborts towards on_disk_prepare_bytes

We have a few tests that try to track on_disk_prepare_size but they
used the cachedVBState which wasn't updated post flush. We shouldn't
count aborts towards on_disk_prepare_bytes as we must keep them for
the metadata purge interval to ensure that replicas can reconnect
correctly. As such their inclusion in on_disk_prepare_bytes would
potentially trigger unnecessary compactions.

Change-Id: Ie42186c538bff6a5bb33dc019e03185aba1079d9
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/143425
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>

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

@@ -2656,26 +2656,29 @@ static void saveDocsCallback(const DocInfo* oldInfo,
         onDiskMutationType = DocMutationType::Insert;
     }
 
+    const ssize_t newSize = newInfo->physical_size;
+    const ssize_t oldSize = oldInfo ? oldInfo->physical_size : 0;
+
     switch (onDiskMutationType) {
     case DocMutationType::Delete:
         if (newKey.isPrepared()) {
             cbCtx->onDiskPrepareDelta--;
+            cbCtx->onDiskPrepareBytesDelta -= oldSize;
         }
         break;
     case DocMutationType::Insert:
         if (newKey.isPrepared()) {
             cbCtx->onDiskPrepareDelta++;
+            cbCtx->onDiskPrepareBytesDelta += newSize;
         }
         break;
     case DocMutationType::Update:
+        if (!newInfo->deleted) {
+            // Not an abort, update the stat
+            cbCtx->onDiskPrepareBytesDelta += (newSize - oldSize);
+        }
         break;
     }
-
-    if (newKey.isPrepared()) {
-        const ssize_t newSize = newInfo->physical_size;
-        const ssize_t oldSize = oldInfo ? oldInfo->physical_size : 0;
-        cbCtx->onDiskPrepareBytesDelta += (newSize - oldSize);
-    }
 }
 
 /**

engines/ep/tests/module_tests/couch-kvstore_test.cc

@@ -1298,6 +1298,27 @@ class CouchstoreTest : public ::testing::Test {
         flush.proposedVBState.transition.state = vbucket_state_active;
     }
 
+    void persistPrepare(std::string key, std::string value, uint64_t seqno) {
+        auto storedDocKey = makeStoredDocKey(key);
+        auto qi = makePendingItem(storedDocKey, value);
+        qi->setBySeqno(seqno);
+
+        kvstore->begin(std::make_unique<TransactionContext>(vbid));
+        kvstore->del(qi);
+        kvstore->commit(flush);
+    }
+
+    void persistAbort(std::string key, std::string value, uint64_t seqno) {
+        auto storedDocKey = makeStoredDocKey(key);
+        auto qi = makePendingItem(storedDocKey, value);
+        qi->setBySeqno(seqno);
+        qi->setAbortSyncWrite();
+
+        kvstore->begin(std::make_unique<TransactionContext>(vbid));
+        kvstore->del(qi);
+        kvstore->commit(flush);
+    }
+
     ~CouchstoreTest() override {
         cb::io::rmrf(data_dir.c_str());
     }
@@ -2191,3 +2212,73 @@ TEST_F(CouchstoreTest, ConcurrentCompactionAndFlushingAbortToAbort) {
     vbstate = kvstore->getPersistedVBucketState(vbid);
     EXPECT_EQ(0, vbstate.onDiskPrepares);
 }
+
+TEST_F(CouchstoreTest, PersistPrepareStats) {
+    persistPrepare("key", "value", 1);
+
+    auto persistedVBState = kvstore->getPersistedVBucketState(vbid);
+    EXPECT_EQ(1, persistedVBState.onDiskPrepares);
+    EXPECT_LT(0, persistedVBState.getOnDiskPrepareBytes());
+
+    auto dbFileInfo = kvstore->getDbFileInfo(vbid);
+    EXPECT_LT(0, dbFileInfo.prepareBytes);
+}
+
+TEST_F(CouchstoreTest, PersistAbortStats) {
+    persistAbort("key", "value", 1);
+
+    auto persistedVBState = kvstore->getPersistedVBucketState(vbid);
+    EXPECT_EQ(0, persistedVBState.onDiskPrepares);
+    EXPECT_EQ(0, persistedVBState.getOnDiskPrepareBytes());
+
+    auto dbFileInfo = kvstore->getDbFileInfo(vbid);
+    EXPECT_EQ(0, dbFileInfo.prepareBytes);
+}
+
+TEST_F(CouchstoreTest, PersistPreparePrepareStats) {
+    persistPrepare("key", "value", 1);
+    persistPrepare("key", "longervalue", 2);
+
+    auto persistedVBState = kvstore->getPersistedVBucketState(vbid);
+    EXPECT_EQ(1, persistedVBState.onDiskPrepares);
+    EXPECT_LT(0, persistedVBState.getOnDiskPrepareBytes());
+
+    auto dbFileInfo = kvstore->getDbFileInfo(vbid);
+    EXPECT_LT(0, dbFileInfo.prepareBytes);
+}
+
+TEST_F(CouchstoreTest, PersistPrepareAbortStats) {
+    persistPrepare("key", "value", 1);
+    persistAbort("key", "differentvalue", 2);
+
+    auto persistedVBState = kvstore->getPersistedVBucketState(vbid);
+    EXPECT_EQ(0, persistedVBState.onDiskPrepares);
+    EXPECT_EQ(0, persistedVBState.getOnDiskPrepareBytes());
+
+    auto dbFileInfo = kvstore->getDbFileInfo(vbid);
+    EXPECT_EQ(0, dbFileInfo.prepareBytes);
+}
+
+TEST_F(CouchstoreTest, PersistAbortPrepareStats) {
+    persistAbort("key", "value", 1);
+    persistPrepare("key", "differentvalue", 2);
+
+    auto persistedVBState = kvstore->getPersistedVBucketState(vbid);
+    EXPECT_EQ(1, persistedVBState.onDiskPrepares);
+    EXPECT_LT(0, persistedVBState.getOnDiskPrepareBytes());
+
+    auto dbFileInfo = kvstore->getDbFileInfo(vbid);
+    EXPECT_LT(0, dbFileInfo.prepareBytes);
+}
+
+TEST_F(CouchstoreTest, PersistAbortAbortStats) {
+    persistAbort("key", "value", 1);
+    persistAbort("key", "differentvalue", 2);
+
+    auto persistedVBState = kvstore->getPersistedVBucketState(vbid);
+    EXPECT_EQ(0, persistedVBState.onDiskPrepares);
+    EXPECT_EQ(0, persistedVBState.getOnDiskPrepareBytes());
+
+    auto dbFileInfo = kvstore->getDbFileInfo(vbid);
+    EXPECT_EQ(0, dbFileInfo.prepareBytes);
+}