MB-42306 [2/2]: Bias db_data_size by estimate of completed prepares

daverigby · daverigby · commit 58937d772653 · 2020-11-05T11:31:06.000Z
+Problem+ The couchstore fragmentation calculation is not taking into account completed Prepares. As such, auto-compaction is not run when expected, and hence the completed (no longer needed) prepares are not purged. This happens because couch_disk_data_size (size of "valid" data on disk) is calculated directly from couchstore's own count of how much data is in the current B-Tree root. While completed prepares can be purged when compaction runs (and hence are conceptually 'stale' data), completed Prepares are conceptually "valid" data from couchstore's POV - they are just documents with a different key prefix which happen to have a seqno below the high_completed_seqno. As such, couch_disk_data_size includes all prepares, outstanding and completed. +Solution+ Ideally KV-Engine would track the total size of all on-disk completed Prepares (whose which can be purged), and subtract that value from the couchstore-tracked couch_disk_data_size. However, that is costly to track accurately - we would have to perform B-Tree seqno scan from 0 to the highCompletedSeqno, accumulating the size of all prepares found, which is an O(N) operation where N = the number of completed prepares. As such it is not suitable for the ~1s polling which ns_server makes. Alternatively (and the approach taken here) we could ignore pending (incomplete) prepares entirely, and assume that all prepares are completed. This is based on the observation that prepares have a maximum of 65s timeout before they are aborted, and most will be Committed much sooner than that. This patch therefore: * Exposes new 'prepare_size' stats, which are the number of bytes of all on-disk prepares: - ep_db_prepare_size (from 'all' or 'diskinfo' group) - vb_XXX:db_prepare_size (from 'vbucket-details' group) - vb_XXX:prepare_size (from 'diskinfo detail' group) * Uses the newly-tracked on_disk_prepare_bytes to bias the raw couchstore-provided couch_disk_data_size stat. This means the following stats are all reduced by the size of the on-disk prepares: - ep_db_data_size (from 'all' or 'diskinfo' group) - vb_XXX:db_data_size (from 'vbucket-details' group) - vb_XXX:data_size (from 'diskinfo detail' group) Change-Id: I4da06fe17a68c96ddd03d027aed9696c5a20def8 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/139312 Tested-by: Dave Rigby <daver@couchbase.com> Reviewed-by: James Harrison <james.harrison@couchbase.com> Well-Formed: Build Bot <build@couchbase.com>
diff --git a/engines/ep/docs/stats.org b/engines/ep/docs/stats.org
@@ -242,6 +242,7 @@ For introductory information on stats within Couchbase, start with the
 |                                       | for this bucket                         |
 | ep_db_data_size                       | Total size of valid data in db files    |
 | ep_db_file_size                       | Total size of the db files              |
+| ep_db_prepare_size                    | Total size of SyncWrite prepares in db files |
 | ep_degraded_mode                      | True if the engine is either warming    |
 |                                       | up or data traffic is disabled          |
 | ep_exp_pager_enabled                  | True if the expiry pager is enabled     |
@@ -560,6 +561,7 @@ The stats below are listed for each vbucket.
 | pending writes                | Total bytes of pending writes              |
 | db_data_size                  | Total size of valid data on disk           |
 | db_file_size                  | Total size of the db file                  |
+| db_prepare_size               | Total size of SyncWrite prepares on disk   |
 | high_seqno                    | The last seqno assigned by this vbucket    |
 | purge_seqno                   | The last seqno purged by the compactor     |
 | bloom_filter                  | Status of the vbucket's bloom filter       |
diff --git a/engines/ep/src/couch-kvstore/couch-kvstore.cc b/engines/ep/src/couch-kvstore/couch-kvstore.cc
@@ -355,6 +355,7 @@ CouchKVStore::CouchKVStore(KVStoreConfig& config,
     cachedDeleteCount.assign(numDbFiles, cb::RelaxedAtomic<size_t>(-1));
     cachedFileSize.assign(numDbFiles, cb::RelaxedAtomic<uint64_t>(0));
     cachedSpaceUsed.assign(numDbFiles, cb::RelaxedAtomic<uint64_t>(0));
+    cachedOnDiskPrepareSize.assign(numDbFiles, 0);
     cachedVBStates.resize(numDbFiles);
 
     initialize();
@@ -472,6 +473,7 @@ void CouchKVStore::reset(Vbid vbucketId) {
         cachedDeleteCount[vbucketId.get()] = 0;
         cachedFileSize[vbucketId.get()] = 0;
         cachedSpaceUsed[vbucketId.get()] = 0;
+        cachedOnDiskPrepareSize[vbucketId.get()] = 0;
 
         // Unlink the current revision and then increment it to ensure any
         // pending delete doesn't delete us. Note that the expectation is that
@@ -1242,6 +1244,7 @@ bool CouchKVStore::compactDBInternal(compaction_ctx* hook_ctx,
         state->onDiskPrepares -= hook_ctx->stats.preparesPurged;
         state->updateOnDiskPrepareBytes(
                 -int64_t(hook_ctx->stats.prepareBytesPurged));
+        cachedOnDiskPrepareSize[vbid.get()] = state->getOnDiskPrepareBytes();
     }
 
     logger.debug("INFO: created new couch db file, name:{} rev:{}",
@@ -2417,6 +2420,7 @@ couchstore_error_t CouchKVStore::saveDocs(Vbid vbid,
         cachedFileSize[vbid.get()] = info.file_size;
         cachedDeleteCount[vbid.get()] = info.deleted_count;
         cachedDocCount[vbid.get()] = info.doc_count;
+        cachedOnDiskPrepareSize[vbid.get()] = state->getOnDiskPrepareBytes();
 
         // Check seqno if we wrote documents
         if (docs.size() > 0 && maxDBSeqno != info.last_sequence) {
@@ -2891,7 +2895,9 @@ size_t CouchKVStore::getNumPersistedDeletes(Vbid vbid) {
 
 DBFileInfo CouchKVStore::getDbFileInfo(Vbid vbid) {
     DbInfo info = getDbInfo(vbid);
-    return DBFileInfo{info.file_size, info.space_used};
+    return DBFileInfo{info.file_size,
+                      info.space_used,
+                      cachedOnDiskPrepareSize[vbid.get()]};
 }
 
 DBFileInfo CouchKVStore::getAggrDbFileInfo() {
@@ -2904,6 +2910,7 @@ DBFileInfo CouchKVStore::getAggrDbFileInfo() {
     for (uint16_t vbid = 0; vbid < numDbFiles; vbid++) {
         kvsFileInfo.fileSize += cachedFileSize[vbid].load();
         kvsFileInfo.spaceUsed += cachedSpaceUsed[vbid].load();
+        kvsFileInfo.prepareBytes += cachedOnDiskPrepareSize[vbid].load();
     }
     return kvsFileInfo;
 }
@@ -3223,6 +3230,7 @@ uint64_t CouchKVStore::prepareToDeleteImpl(Vbid vbid) {
     cachedDeleteCount[vbid.get()] = 0;
     cachedFileSize[vbid.get()] = 0;
     cachedSpaceUsed[vbid.get()] = 0;
+    cachedOnDiskPrepareSize[vbid.get()] = 0;
     return getDbRevision(vbid);
 }
 
diff --git a/engines/ep/src/couch-kvstore/couch-kvstore.h b/engines/ep/src/couch-kvstore/couch-kvstore.h
@@ -828,6 +828,9 @@ class CouchKVStore : public KVStore
     std::vector<cb::RelaxedAtomic<size_t>> cachedDeleteCount;
     std::vector<cb::RelaxedAtomic<uint64_t>> cachedFileSize;
     std::vector<cb::RelaxedAtomic<uint64_t>> cachedSpaceUsed;
+    /// Size of on-disk prepares, indexed by vBucket RelaxedAtomic to allow
+    /// stats access without lock.
+    std::vector<cb::RelaxedAtomic<size_t>> cachedOnDiskPrepareSize;
 
     /* pending file deletions */
     folly::Synchronized<std::queue<std::string>> pendingFileDeletions;
diff --git a/engines/ep/src/ep_bucket.cc b/engines/ep/src/ep_bucket.cc
@@ -1219,10 +1219,16 @@ ENGINE_ERROR_CODE EPBucket::getFileStats(const void* cookie,
                 getRWUnderlyingByShard(shardId)->getAggrDbFileInfo();
         totalInfo.spaceUsed += dbInfo.spaceUsed;
         totalInfo.fileSize += dbInfo.fileSize;
+        totalInfo.prepareBytes += dbInfo.prepareBytes;
     }
 
-    add_casted_stat("ep_db_data_size", totalInfo.spaceUsed, add_stat, cookie);
+    add_casted_stat("ep_db_data_size",
+                    totalInfo.getEstimatedLiveData(),
+                    add_stat,
+                    cookie);
     add_casted_stat("ep_db_file_size", totalInfo.fileSize, add_stat, cookie);
+    add_casted_stat(
+            "ep_db_prepare_size", totalInfo.prepareBytes, add_stat, cookie);
 
     return ENGINE_SUCCESS;
 }
@@ -1244,10 +1250,14 @@ ENGINE_ERROR_CODE EPBucket::getPerVBucketDiskStats(const void* cookie,
 
                 checked_snprintf(
                         buf, sizeof(buf), "vb_%d:data_size", vbid.get());
-                add_casted_stat(buf, dbInfo.spaceUsed, add_stat, cookie);
+                add_casted_stat(
+                        buf, dbInfo.getEstimatedLiveData(), add_stat, cookie);
                 checked_snprintf(
                         buf, sizeof(buf), "vb_%d:file_size", vbid.get());
                 add_casted_stat(buf, dbInfo.fileSize, add_stat, cookie);
+                checked_snprintf(
+                        buf, sizeof(buf), "vb_%d:prepare_size", vbid.get());
+                add_casted_stat(buf, dbInfo.prepareBytes, add_stat, cookie);
             } catch (std::exception& error) {
                 EP_LOG_WARN(
                         "DiskStatVisitor::visitBucket: Failed to build stat: "
@@ -1944,4 +1954,4 @@ bool EPBucket::isValidBucketDurabilityLevel(cb::durability::Level level) const {
         return true;
     }
     folly::assume_unreachable();
-}
+}
diff --git a/engines/ep/src/ep_vb.cc b/engines/ep/src/ep_vb.cc
@@ -422,16 +422,12 @@ void EPVBucket::addStats(VBucketStatsDetailLevel detail,
     _addStats(detail, add_stat, c);
 
     if (detail == VBucketStatsDetailLevel::Full) {
-        uint64_t spaceUsed = 0;
-        uint64_t fileSize = 0;
+        DBFileInfo fileInfo;
 
         // Only try to read disk if we believe the file has been created
         if (!isBucketCreation()) {
             try {
-                DBFileInfo fileInfo =
-                        shard->getRWUnderlying()->getDbFileInfo(getId());
-                spaceUsed = fileInfo.spaceUsed;
-                fileSize = fileInfo.fileSize;
+                fileInfo = shard->getRWUnderlying()->getDbFileInfo(getId());
             } catch (std::runtime_error& e) {
                 EP_LOG_WARN(
                         "VBucket::addStats: Exception caught during "
@@ -441,8 +437,9 @@ void EPVBucket::addStats(VBucketStatsDetailLevel detail,
                         e.what());
             }
         }
-        addStat("db_data_size", spaceUsed, add_stat, c);
-        addStat("db_file_size", fileSize, add_stat, c);
+        addStat("db_data_size", fileInfo.getEstimatedLiveData(), add_stat, c);
+        addStat("db_file_size", fileInfo.fileSize, add_stat, c);
+        addStat("db_prepare_size", fileInfo.prepareBytes, add_stat, c);
     }
 }
 
diff --git a/engines/ep/src/kvstore.h b/engines/ep/src/kvstore.h
@@ -145,14 +145,43 @@ class NoLookupCallback : public StatusCallback<CacheLookup> {
 };
 
 struct DBFileInfo {
-    DBFileInfo() :
-        fileSize(0), spaceUsed(0) { }
+    /// Total size of the file (what 'stat()' would return). Includes both
+    /// current data (spaceUsed) plus any previous data which is no longer
+    /// referenced in current file header.
+    uint64_t fileSize = 0;
+
+    /// Total size of "current" data in the file - sum of all
+    /// keys+metdata+values (included deleted docs) plus overheads to manage it
+    /// (indexes such as B-Trees, headers etc).
+    uint64_t spaceUsed = 0;
+
+    /// Total size of all SyncWrite prepares, both completed and pending.
+    /// This can be used to adjust spaceUsed to give an estimate of how much
+    /// data in the file is actually needed - completed prepares are no
+    /// longer needed and can be purged during compaction - as such they can
+    /// be considered part of the "Fragmented" count.
+    uint64_t prepareBytes = 0;
 
-    DBFileInfo(uint64_t fileSize_, uint64_t spaceUsed_)
-        : fileSize(fileSize_), spaceUsed(spaceUsed_) {}
-
-    uint64_t fileSize;
-    uint64_t spaceUsed;
+    /**
+     * @returns An estimate of the number of bytes which are "live" data and
+     * hence are not subject to being discarded during compactionn. This
+     * is calculated as the size of the current data (spaceUsed), minus an
+     * estimate of the size of completed prepares (which will be purged on
+     * compaction).
+     * Note: All prepared SyncWrites (completed and in-progress) are used as
+     *       an estimate for completed sync writes, given (a) it's difficult
+     *       to track exactly how any prepares have been completed and (b)
+     *       in general we expect the overwhelming majority of on-disk prepares
+     *       to be completed.
+     */
+    uint64_t getEstimatedLiveData() const {
+        if (spaceUsed > prepareBytes) {
+            // Sanity check - if totalOnDiskPrepareSize is somehow larger than
+            // spaceUsed then skip the adjustment.
+            return spaceUsed - prepareBytes;
+        }
+        return spaceUsed;
+    }
 };
 
 enum scan_error_t {
diff --git a/engines/ep/tests/ep_testsuite.cc b/engines/ep/tests/ep_testsuite.cc
@@ -7512,9 +7512,11 @@ static enum test_result test_mb19687_fixed(EngineIface* h) {
         // Using explicit initializer lists due to http://stackoverflow
         // .com/questions/36557969/invalid-iterator-range-while-inserting
         // -initializer-list-to-an-stdvector
-        eng_stats.insert(eng_stats.end(),
-                         std::initializer_list<std::string>{"ep_db_data_size",
-                                                            "ep_db_file_size"});
+        eng_stats.insert(
+                eng_stats.end(),
+                std::initializer_list<std::string>{"ep_db_data_size",
+                                                   "ep_db_file_size",
+                                                   "ep_db_prepare_size"});
         eng_stats.insert(eng_stats.end(),
                          std::initializer_list<std::string>{"ep_flusher_state",
                                                             "ep_flusher_todo"});
@@ -7544,8 +7546,10 @@ static enum test_result test_mb19687_fixed(EngineIface* h) {
         eng_stats.insert(eng_stats.end(), persistentConfig);
 
         // 'diskinfo and 'diskinfo detail' keys should be present now.
-        statsKeys["diskinfo"] = {"ep_db_data_size", "ep_db_file_size"};
-        statsKeys["diskinfo detail"] = {"vb_0:data_size", "vb_0:file_size"};
+        statsKeys["diskinfo"] = {
+                "ep_db_data_size", "ep_db_file_size", "ep_db_prepare_size"};
+        statsKeys["diskinfo detail"] = {
+                "vb_0:data_size", "vb_0:file_size", "vb_0:prepare_size"};
 
         // Add stats which are only available for persistent buckets:
         static const char* persistence_stats[] = {
@@ -7562,6 +7566,7 @@ static enum test_result test_mb19687_fixed(EngineIface* h) {
         auto& vb_details = statsKeys.at("vbucket-details 0");
         vb_details.push_back("vb_0:db_data_size");
         vb_details.push_back("vb_0:db_file_size");
+        vb_details.push_back("vb_0:db_prepare_size");
 
         // Config variables only valid for persistent
         auto& config_stats = statsKeys.at("config");
