diff --git a/engines/ep/src/couch-kvstore/couch-kvstore.cc b/engines/ep/src/couch-kvstore/couch-kvstore.cc index f6d1c27533..f7cfe81528 100644 --- a/engines/ep/src/couch-kvstore/couch-kvstore.cc +++ b/engines/ep/src/couch-kvstore/couch-kvstore.cc @@ -1082,11 +1082,7 @@ couchstore_error_t CouchKVStore::replayPrecommitHook( } try { - if (std::stoul(json["on_disk_prepares"].get()) != - prepareStats.onDiskPrepares) { - json["on_disk_prepares"] = - std::to_string(prepareStats.onDiskPrepares); - } + json["on_disk_prepares"] = std::to_string(prepareStats.onDiskPrepares); auto prepareBytes = json.find("on_disk_prepare_bytes"); // only update if it's there.. @@ -1377,12 +1373,15 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal( }, {}, def_iops, - [vbid, hook_ctx, this](Db& compacted) { + [vbid, hook_ctx, this](Db& source, Db& compacted) { // we don't try to delete the dropped collection document // as it'll come back in the next database header anyway PendingLocalDocRequestQueue localDocQueue; - auto ret = maybePatchMetaData( - compacted, hook_ctx->stats, localDocQueue, vbid); + auto ret = maybePatchMetaData(source, + compacted, + hook_ctx->stats, + localDocQueue, + vbid); if (ret == COUCHSTORE_SUCCESS) { ret = updateLocalDocuments(compacted, localDocQueue); } @@ -1425,8 +1424,8 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal( target, docInfo, prepareStats, collectionStats); }, [&prepareStats, &collectionStats, &purge_seqno, hook_ctx, this]( - Db& db) { - return replayPrecommitHook(db, + Db&, Db& compacted) { + return replayPrecommitHook(compacted, prepareStats, collectionStats, purge_seqno, @@ -1466,7 +1465,7 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal( }, {}, def_iops, - [vbid, hook_ctx, this](Db& compacted) { + [vbid, hook_ctx, this](Db& source, Db& compacted) { if (mb40415_regression_hook) { return COUCHSTORE_ERROR_CANCEL; } @@ -1481,8 +1480,11 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal( hook_ctx->eraserContext->setOnDiskDroppedDataExists( false); } - auto ret = maybePatchMetaData( - compacted, hook_ctx->stats, localDocQueue, vbid); + auto ret = maybePatchMetaData(source, + compacted, + hook_ctx->stats, + localDocQueue, + vbid); if (ret == COUCHSTORE_SUCCESS) { ret = updateLocalDocuments(compacted, localDocQueue); } @@ -1530,8 +1532,11 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal( couchstore_strerror(status)); return CompactDBInternalStatus::Failed; } + + // This field may not be present if first compaction after upgrade from + // less than 6.5 to 7.0, e.g. 6.0 -> 7.0 prepareStats.onDiskPrepares = - std::stoul(json["on_disk_prepares"].get()); + std::stoul(json.value("on_disk_prepares", "0")); auto prepareBytes = json.find("on_disk_prepare_bytes"); // only update if it's there.. @@ -1697,7 +1702,8 @@ bool CouchKVStore::compactDBTryAndSwitchToNewFile( } couchstore_error_t CouchKVStore::maybePatchMetaData( - Db& db, + Db& source, + Db& target, CompactionStats& stats, PendingLocalDocRequestQueue& localDocQueue, Vbid vbid) { @@ -1706,7 +1712,7 @@ couchstore_error_t CouchKVStore::maybePatchMetaData( // Must sync the modified state back we need to update the // _local/vbstate to update the number of on_disk_prepares to match // whatever we purged - auto [status, json] = getLocalVbState(db); + auto [status, json] = getLocalVbState(target); if (status != COUCHSTORE_SUCCESS) { logger.warn( "CouchKVStore::maybePatchMetaData: Failed to load " @@ -1770,8 +1776,11 @@ couchstore_error_t CouchKVStore::maybePatchMetaData( // Finally see if compaction purged data and disk size stats need updates for (auto [cid, purgedBytes] : stats.collectionSizeUpdates) { - // Need to read the collection stats - auto [success, currentStats] = getCollectionStats(db, cid); + // Need to read the collection stats. We read from the source file so + // that in the case of a first compaction after upgrade from pre 7.0 + // we get the correct disk (because we use the dbinfo.space_used). The + // target file could return a value too small in that case (MB-45917) + auto [success, currentStats] = getCollectionStats(source, cid); if (!success) { logger.warn( "CouchKVStore::maybePatchMetaData: Failed to load " @@ -1799,7 +1808,6 @@ couchstore_error_t CouchKVStore::maybePatchMetaData( return COUCHSTORE_SUCCESS; } - bool CouchKVStore::tryToCatchUpDbFile(Db& source, Db& destination, std::unique_lock& lock, @@ -1887,7 +1895,7 @@ bool CouchKVStore::tryToCatchUpDbFile(Db& source, target, docInfo, prepareStats, collectionStats); }, [&prepareStats, &collectionStats, &purge_seqno, &hook_ctx, this]( - Db& compacted) { + Db&, Db& compacted) { return replayPrecommitHook(compacted, prepareStats, collectionStats, diff --git a/engines/ep/src/couch-kvstore/couch-kvstore.h b/engines/ep/src/couch-kvstore/couch-kvstore.h index b2eab33a6d..26ebb5f878 100644 --- a/engines/ep/src/couch-kvstore/couch-kvstore.h +++ b/engines/ep/src/couch-kvstore/couch-kvstore.h @@ -772,13 +772,15 @@ class CouchKVStore : public KVStore * The collection stats may need updating if documents in collections were * purged. * - * @param db The compacted database + * @param source The source of compaction + * @param target The compacted database * @param stats Data tracked by compaction used for stat updates * @param localDocQueue the queue which will be updated with new documents * @param vbid vbucket being compacted */ couchstore_error_t maybePatchMetaData( - Db& db, + Db& source, + Db& target, CompactionStats& stats, PendingLocalDocRequestQueue& localDocQueue, Vbid vbid); diff --git a/engines/ep/tests/module_tests/couch-kvstore_test.cc b/engines/ep/tests/module_tests/couch-kvstore_test.cc index 3cf368bcdd..39f52f7d5d 100644 --- a/engines/ep/tests/module_tests/couch-kvstore_test.cc +++ b/engines/ep/tests/module_tests/couch-kvstore_test.cc @@ -42,7 +42,24 @@ class CouchKVStoreTest : public KVStoreTest { CouchKVStoreTest() : KVStoreTest() { } - void collectionsOfflineUpgrade(bool writeAsMadHatter); + /** + * Run an offline upgrade + * @param writeAsMadHatter Make the data file appear as if it's 6.5 + * @param outputCid Upgrade and move the keys to this collection + * @param keys how many keys to write + * @param deleteKeyRange a range of keys that will be deleted + */ + void collectionsOfflineUpgrade(bool writeAsMadHatter, + CollectionID outputCid, + int keys, + std::pair deletedKeyRange); + + void runCompaction(KVStore& kvstore, CompactionConfig config) { + std::mutex mutex; + std::unique_lock lock(mutex); + auto ctx = std::make_shared(Vbid(0), config, 0); + EXPECT_TRUE(kvstore.compactDB(lock, ctx)); + } }; // Verify the stats returned from operations are accurate. @@ -173,8 +190,9 @@ class CollectionsOfflineUpgradeCallback : public StatusCallback { class CollectionsOfflineGetCallback : public StatusCallback { public: - explicit CollectionsOfflineGetCallback(std::pair deletedRange) - : deletedRange(std::move(deletedRange)) { + explicit CollectionsOfflineGetCallback(CollectionID expectedId, + std::pair deletedRange) + : expectedId(expectedId), deletedRange(std::move(deletedRange)) { } void callback(GetValue& result) override { @@ -182,9 +200,8 @@ class CollectionsOfflineGetCallback : public StatusCallback { if (result.item->isDeleted()) { DocKey dk = result.item->getKey(); - EXPECT_EQ(500, dk.getCollectionID()); + EXPECT_EQ(expectedId, dk.getCollectionID()); auto noCollection = dk.makeDocKeyWithoutCollectionID(); - EXPECT_EQ(3, noCollection.size()); // create a string from the logical-key, i.e. +1 and skip the // collection-ID std::string str( @@ -206,33 +223,38 @@ class CollectionsOfflineGetCallback : public StatusCallback { result.item->getDataType()); result.item->decompressValue(); - EXPECT_EQ(0, - strncmp("valuable", - result.item->getData(), - result.item->getNBytes())); + std::string_view value{result.item->getData(), + result.item->getNBytes()}; + + std::string expectedValue = "valuable"; + if (result.item->getDataType() & PROTOCOL_BINARY_DATATYPE_XATTR) { + expectedValue = createXattrValue("value"); + } + EXPECT_EQ(expectedValue, value); } private: + CollectionID expectedId; std::pair deletedRange; }; // Test the InputCouchFile/OutputCouchFile objects (in a simple test) to // check they do what we expect, that is create a new couchfile with all keys // moved into a specified collection. -void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) { +void CouchKVStoreTest::collectionsOfflineUpgrade( + bool writeAsMadHatter, + CollectionID outputCid, + int keys, + std::pair deletedRange) { + ASSERT_LE(deletedRange.first, deletedRange.second); + ASSERT_LE(deletedRange.second, keys); CouchKVStoreConfig config1(1024, 4, data_dir, "couchdb", 0); - CouchKVStoreConfig config2(1024, 4, data_dir, "couchdb", 0); // Test setup, create a new file auto kvstore = setup_kv_store(config1); kvstore->begin(std::make_unique(vbid)); - // The key count is large enough to ensure the count uses more than 1 byte - // of leb storage so we validate that leb encode/decode works on this path - const int keys = 129; - const int deletedKeys = 14; - for (int i = 0; i < keys; i++) { // key needs to look like it's in the default collection so we can flush // it @@ -248,25 +270,26 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) { kvstore->begin(std::make_unique(vbid)); - // Delete some keys. With and without a value (like xattr) - for (int i = 18, j = 1; i < 18 + deletedKeys; ++i, ++j) { + // Now delete keys (if requested). Alternate between value and no-value + // (like xattr) + for (int i = deletedRange.first, seqno = keys; i < deletedRange.second; + ++i, ++seqno) { std::unique_ptr item; auto key = makeStoredDocKey(std::to_string(i)); if (i & 1) { item.reset(Item::makeDeletedItem( DeleteSource::Explicit, key, 0, 0, nullptr, 0)); } else { - // Note: this is not really xattr, just checking the datatype is - // preserved on upgrade + const auto body = createXattrValue("value"); item.reset(Item::makeDeletedItem(DeleteSource::Explicit, key, 0, 0, - "valuable", - 8, + body.data(), + body.size(), PROTOCOL_BINARY_DATATYPE_XATTR)); } - item->setBySeqno(keys + j); + item->setBySeqno(seqno); kvstore->del(queued_item(std::move(item))); } kvstore->commit(flush); @@ -276,7 +299,7 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) { // Use the upgrade tool's objects to run an upgrade // setup_kv_store will have progressed the rev to .2 Collections::InputCouchFile input({}, data_dir + "/0.couch.2"); - CollectionID cid = 500; + CollectionID cid = outputCid; Collections::OutputCouchFile output({}, data_dir + "/0.couch.3", cid /*collection-id*/, @@ -291,8 +314,7 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) { auto kvstore2 = KVStoreFactory::create(config2); auto scanCtx = kvstore2.rw->initBySeqnoScanContext( - std::make_unique( - std::pair{18, 18 + deletedKeys}), + std::make_unique(cid, deletedRange), std::make_unique(cid), Vbid(0), 1, @@ -312,17 +334,37 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) { auto [success, stats] = kvstore2.rw->getCollectionStats(*kvstoreContext, cid); EXPECT_TRUE(success); - EXPECT_EQ(keys - deletedKeys, stats.itemCount); - EXPECT_EQ(keys + deletedKeys, stats.highSeqno); + auto deletedCount = deletedRange.second - deletedRange.first; + EXPECT_EQ(keys - deletedCount, stats.itemCount); + EXPECT_EQ(keys + (deletedCount - 1), stats.highSeqno); EXPECT_NE(0, stats.diskSize); + + // Test that the datafile can be compacted. + // MB-45917 + CompactionConfig config{0, 0, true /*purge all tombstones*/, false}; + runCompaction(*kvstore2.rw, config); } TEST_F(CouchKVStoreTest, CollectionsOfflineUpgrade) { - collectionsOfflineUpgrade(false); + // The key count is large enough to ensure the count uses more than 1 byte + // of leb storage so we validate that leb encode/decode works on this path + collectionsOfflineUpgrade(false, CollectionID{500}, 129, {14, 14 + 18}); } TEST_F(CouchKVStoreTest, CollectionsOfflineUpgradeMadHatter) { - collectionsOfflineUpgrade(true); + // The key count is large enough to ensure the count uses more than 1 byte + // of leb storage so we validate that leb encode/decode works on this path + collectionsOfflineUpgrade(true, CollectionID{500}, 129, {14, 14 + 18}); +} + +TEST_F(CouchKVStoreTest, CollectionsOfflineUpgradeMadHatter_MB_45917) { + // Delete all the keys, this would trigger MB-45917. + // Here we upgrade as if from mad-hatter, that means there is no diskSize + // stat in the collections local doc, which then leads to an underflow as + // the wrong file disk size was used in calculating the collection's disk + // size. The target collection could be anything, but do a to=default so + // it more mimics what happens. Finally write 129 keys and delete them all. + collectionsOfflineUpgrade(true, CollectionID::Default, 129, {0, 129}); } TEST_F(CouchKVStoreTest, OpenHistoricalSnapshot) {