diff --git a/engines/ep/src/collections/flush_accounting.cc b/engines/ep/src/collections/flush_accounting.cc index 06c60db4a5..edec4b7662 100644 --- a/engines/ep/src/collections/flush_accounting.cc +++ b/engines/ep/src/collections/flush_accounting.cc @@ -15,13 +15,6 @@ namespace Collections::VB { -// Returns two optionals -// First is only initialised if the key is a collection system event. -// Second is only initialised if there is a collection id present. -// Any mutation will return nullopt,cid -// A collection create/drop/modify will return event, cid -// A scope event will return nullopt, nullopt - /** * From a DocKey extract CollectionID and SystemEvent data. * The function returns two optionals because depending on the key there could diff --git a/engines/ep/src/collections/scan_context.cc b/engines/ep/src/collections/scan_context.cc index 4620f834bd..3c8d0174b4 100644 --- a/engines/ep/src/collections/scan_context.cc +++ b/engines/ep/src/collections/scan_context.cc @@ -12,6 +12,7 @@ #include "collections/scan_context.h" #include "collections/kvstore.h" +#include "systemevent_factory.h" namespace Collections::VB { @@ -29,13 +30,23 @@ ScanContext::ScanContext( } bool ScanContext::isLogicallyDeleted(const DocKey& key, uint64_t seqno) const { - if (dropped.empty() || key.isInSystemCollection()) { + if (dropped.empty()) { return false; } + CollectionID cid; + if (key.isInSystemCollection()) { + auto modify = SystemEventFactory::isModifyCollection(key); + if (!modify) { + return false; + } + cid = modify.value(); + } else { + cid = key.getCollectionID(); + } + // Is the key in a range which contains dropped collections and in the set? - return (seqno >= startSeqno && seqno <= endSeqno) && - dropped.count(key.getCollectionID()) > 0; + return (seqno >= startSeqno && seqno <= endSeqno) && dropped.count(cid) > 0; } std::ostream& operator<<(std::ostream& os, const ScanContext& scanContext) { diff --git a/engines/ep/src/ep_bucket.cc b/engines/ep/src/ep_bucket.cc index b3ae576df2..f1e9ce3204 100644 --- a/engines/ep/src/ep_bucket.cc +++ b/engines/ep/src/ep_bucket.cc @@ -1268,7 +1268,8 @@ void EPBucket::dropKey(VBucket& vb, auto docKey = diskKey.getDocKey(); if (docKey.isInSystemCollection()) { - throw std::logic_error("EPBucket::dropKey called for a system key"); + // SystemEvents aren't in memory so return. + return; } if (diskKey.isPrepared() && bySeqno > highCompletedSeqno) { diff --git a/engines/ep/src/ep_types.h b/engines/ep/src/ep_types.h index 5ef290f829..1ae15a17ad 100644 --- a/engines/ep/src/ep_types.h +++ b/engines/ep/src/ep_types.h @@ -310,6 +310,77 @@ using VBucketStateLockRef = cb::SharedLockRef; template using VBucketStateLockMap = folly::F14FastMap; +/** + * Properties of the storage layer. + */ +class StorageProperties { +public: + enum class ByIdScan : bool { Yes, No }; + + /** + * Will the KVStore de-dupe items such that only the highest seqno for any + * given key in a single flush batch is persisted? + */ + enum class AutomaticDeduplication : bool { Yes, No }; + + /** + * Will the KVStore count items in the prepare namespace (and update the + * values appropriately in the vbstate) + */ + enum class PrepareCounting : bool { Yes, No }; + + /** + * Will the KVStore make callbacks with stale (superseded) items during + * compaction? + */ + enum class CompactionStaleItemCallbacks : bool { Yes, No }; + + /** + * Does the KVStore support history retention (suitable for change streams) + */ + enum class HistoryRetentionAvailable : bool { Yes, No }; + + StorageProperties(ByIdScan byIdScan, + AutomaticDeduplication automaticDeduplication, + PrepareCounting prepareCounting, + CompactionStaleItemCallbacks compactionStaleItemCallbacks, + HistoryRetentionAvailable historyRetentionAvailable) + : byIdScan(byIdScan), + automaticDeduplication(automaticDeduplication), + prepareCounting(prepareCounting), + compactionStaleItemCallbacks(compactionStaleItemCallbacks), + historyRetentionAvailable(historyRetentionAvailable) { + } + + bool hasByIdScan() const { + return byIdScan == ByIdScan::Yes; + } + + bool hasAutomaticDeduplication() const { + return automaticDeduplication == AutomaticDeduplication::Yes; + } + + bool hasPrepareCounting() const { + return prepareCounting == PrepareCounting::Yes; + } + + bool hasCompactionStaleItemCallbacks() const { + return compactionStaleItemCallbacks == + CompactionStaleItemCallbacks::Yes; + } + + bool canRetainHistory() const { + return historyRetentionAvailable == HistoryRetentionAvailable::Yes; + } + +private: + ByIdScan byIdScan; + AutomaticDeduplication automaticDeduplication; + PrepareCounting prepareCounting; + CompactionStaleItemCallbacks compactionStaleItemCallbacks; + HistoryRetentionAvailable historyRetentionAvailable; +}; + namespace cb { /** diff --git a/engines/ep/src/ephemeral_vb.cc b/engines/ep/src/ephemeral_vb.cc index d14f2890aa..8236a0a26b 100644 --- a/engines/ep/src/ephemeral_vb.cc +++ b/engines/ep/src/ephemeral_vb.cc @@ -318,10 +318,16 @@ std::optional EphemeralVBucket::makeRangeIterator( bool EphemeralVBucket::isKeyLogicallyDeleted(const DocKey& key, int64_t bySeqno) const { - if (key.isInSystemCollection()) { + if (key.isInSystemCollection() && + !SystemEventFactory::isModifyCollection(key)) { return false; + // else Modify events can be dropped } - auto cHandle = lockCollections(key); + + // ReadLock the manifest and permit system keys - this ensures a Modify + // can lookup the correct collection + auto cHandle = + manifest->lock(key, Collections::VB::Manifest::AllowSystemKeys{}); return !cHandle.valid() || cHandle.isLogicallyDeleted(bySeqno); } @@ -939,11 +945,6 @@ size_t EphemeralVBucket::getNumPersistedDeletes() const { } void EphemeralVBucket::dropKey(const DocKey& key, int64_t bySeqno) { - // The system event doesn't get dropped here (tombstone purger will deal) - if (key.isInSystemCollection()) { - return; - } - // if there is a pending item which will need removing from the DM, // store its seqno here for use after the HT lock has been released. int64_t prepareSeqno = 0; diff --git a/engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc b/engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc index d629a5e600..c22c113c49 100644 --- a/engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc +++ b/engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc @@ -941,7 +941,9 @@ static int time_purge_hook(Db& d, // Track nothing for the individual collection as the stats doc has // already been deleted. - } else { + } else if (!docKey.isInSystemCollection()) { + // item in a collection was dropped, track how many for item count + // adjustment. if (!info->deleted) { ctx.stats.collectionsItemsPurged++; } else { @@ -2823,18 +2825,17 @@ static int bySeqnoScanCallback(Db* db, DocInfo* docinfo, void* ctx) { auto diskKey = makeDiskDocKey(docinfo->id); - // Determine if the key is logically deleted, if it is we skip the key - // Note that system event keys (like create scope) are never skipped here + // Determine if the key is logically deleted auto docKey = diskKey.getDocKey(); - if (!docKey.isInSystemCollection()) { - if (sctx->docFilter != - DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) { - if (sctx->collectionsContext.isLogicallyDeleted(docKey, byseqno)) { - sctx->lastReadSeqno = byseqno; - return COUCHSTORE_SUCCESS; - } + if (sctx->docFilter != DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) { + if (sctx->collectionsContext.isLogicallyDeleted(docKey, byseqno)) { + sctx->lastReadSeqno = byseqno; + return COUCHSTORE_SUCCESS; } + } + // Only do cache lookup for non-system events + if (!docKey.isInSystemCollection()) { CacheLookup lookup(diskKey, byseqno, vbucketId); cl.callback(lookup); diff --git a/engines/ep/src/kvstore/kvstore.cc b/engines/ep/src/kvstore/kvstore.cc index 812c90a4fe..9f9a0c2c29 100644 --- a/engines/ep/src/kvstore/kvstore.cc +++ b/engines/ep/src/kvstore/kvstore.cc @@ -54,7 +54,7 @@ ScanContext::ScanContext( std::unique_ptr> cl, const std::vector& droppedCollections, - int64_t maxSeqno) + uint64_t maxSeqno) : vbid(vbid), handle(std::move(handle)), docFilter(docFilter), @@ -73,8 +73,8 @@ BySeqnoScanContext::BySeqnoScanContext( std::unique_ptr> cl, Vbid vb, std::unique_ptr handle, - int64_t start, - int64_t end, + uint64_t start, + uint64_t end, uint64_t purgeSeqno, DocumentFilter _docFilter, ValueFilter _valFilter, @@ -110,7 +110,7 @@ ByIdScanContext::ByIdScanContext( ValueFilter _valFilter, const std::vector& droppedCollections, - int64_t maxSeqno) + uint64_t maxSeqno) : ScanContext(vb, std::move(handle), _docFilter, diff --git a/engines/ep/src/kvstore/kvstore.h b/engines/ep/src/kvstore/kvstore.h index 409b488c56..5df7c5d832 100644 --- a/engines/ep/src/kvstore/kvstore.h +++ b/engines/ep/src/kvstore/kvstore.h @@ -388,7 +388,7 @@ class ScanContext { std::unique_ptr> cl, const std::vector& droppedCollections, - int64_t maxSeqno); + uint64_t maxSeqno); virtual ~ScanContext() = default; @@ -409,13 +409,13 @@ class ScanContext { } const Vbid vbid; - int64_t lastReadSeqno{0}; + uint64_t lastReadSeqno{0}; std::unique_ptr handle; const DocumentFilter docFilter; const ValueFilter valFilter; BucketLogger* logger; const Collections::VB::ScanContext collectionsContext; - int64_t maxSeqno; + uint64_t maxSeqno; /** * Cumulative count of bytes read from disk during this scan. Counts @@ -441,8 +441,8 @@ class BySeqnoScanContext : public ScanContext { std::unique_ptr> cl, Vbid vb, std::unique_ptr handle, - int64_t start, - int64_t end, + uint64_t start, + uint64_t end, uint64_t purgeSeqno, DocumentFilter _docFilter, ValueFilter _valFilter, @@ -452,7 +452,7 @@ class BySeqnoScanContext : public ScanContext { droppedCollections, std::optional timestamp = {}); - const int64_t startSeqno; + const uint64_t startSeqno; const uint64_t purgeSeqno; const uint64_t documentCount; @@ -512,7 +512,7 @@ class ByIdScanContext : public ScanContext { ValueFilter _valFilter, const std::vector& droppedCollections, - int64_t maxSeqno); + uint64_t maxSeqno); std::vector ranges; // Key should be set by KVStore when a scan must be paused, this is where // a scan can resume from @@ -650,83 +650,6 @@ class KVStoreStats { } }; -/** - * Properties of the storage layer. - * - * If concurrent filesystem access is possible, maxConcurrency() will - * be greater than one. One will need to determine whether more than - * one writer is possible as well as whether more than one reader is - * possible. - */ -class StorageProperties { -public: - enum class ByIdScan : bool { Yes, No }; - - /** - * Will the KVStore de-dupe items such that only the highest seqno for any - * given key in a single flush batch is persisted? - */ - enum class AutomaticDeduplication : bool { Yes, No }; - - /** - * Will the KVStore count items in the prepare namespace (and update the - * values appropriately in the vbstate) - */ - enum class PrepareCounting : bool { Yes, No }; - - /** - * Will the KVStore make callbacks with stale (superseded) items during - * compaction? - */ - enum class CompactionStaleItemCallbacks : bool { Yes, No }; - - /** - * Does the KVStore support history retention (suitable for change streams) - */ - enum class HistoryRetentionAvailable : bool { Yes, No }; - - StorageProperties(ByIdScan byIdScan, - AutomaticDeduplication automaticDeduplication, - PrepareCounting prepareCounting, - CompactionStaleItemCallbacks compactionStaleItemCallbacks, - HistoryRetentionAvailable historyRetentionAvailable) - : byIdScan(byIdScan), - automaticDeduplication(automaticDeduplication), - prepareCounting(prepareCounting), - compactionStaleItemCallbacks(compactionStaleItemCallbacks), - historyRetentionAvailable(historyRetentionAvailable) { - } - - bool hasByIdScan() const { - return byIdScan == ByIdScan::Yes; - } - - bool hasAutomaticDeduplication() const { - return automaticDeduplication == AutomaticDeduplication::Yes; - } - - bool hasPrepareCounting() const { - return prepareCounting == PrepareCounting::Yes; - } - - bool hasCompactionStaleItemCallbacks() const { - return compactionStaleItemCallbacks == - CompactionStaleItemCallbacks::Yes; - } - - bool canRetainHistory() const { - return historyRetentionAvailable == HistoryRetentionAvailable::Yes; - } - -private: - ByIdScan byIdScan; - AutomaticDeduplication automaticDeduplication; - PrepareCounting prepareCounting; - CompactionStaleItemCallbacks compactionStaleItemCallbacks; - HistoryRetentionAvailable historyRetentionAvailable; -}; - - /** * Base class for some KVStores that implements common functionality. */ diff --git a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc index 23696434bb..c20e53f00f 100644 --- a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc +++ b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc @@ -2110,29 +2110,25 @@ MagmaScanResult MagmaKVStore::scanOne( return MagmaScanResult::Next(); } - // Determine if the key is logically deleted, if it is we skip the key - // Note that system event keys (like create scope) are never skipped - // here - if (!lookup.getKey().getDocKey().isInSystemCollection()) { - if (ctx.docFilter != - DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) { - if (ctx.collectionsContext.isLogicallyDeleted( - lookup.getKey().getDocKey(), seqno)) { - ctx.lastReadSeqno = seqno; - if (logger->should_log(spdlog::level::TRACE)) { - logger->TRACE( - "MagmaKVStore::scanOne SKIPPED(Collection " - "Deleted) {} " - "key:{} " - "seqno:{}", - ctx.vbid, - cb::UserData{lookup.getKey().to_string()}, - seqno); - } - return MagmaScanResult::Next(); + // Determine if the key is logically deleted before trying cache/disk read + if (ctx.docFilter != DocumentFilter::ALL_ITEMS_AND_DROPPED_COLLECTIONS) { + if (ctx.collectionsContext.isLogicallyDeleted( + lookup.getKey().getDocKey(), seqno)) { + ctx.lastReadSeqno = seqno; + if (logger->should_log(spdlog::level::TRACE)) { + logger->TRACE( + "MagmaKVStore::scanOne SKIPPED(Collection " + "Deleted) {} key:{} seqno:{}", + ctx.vbid, + cb::UserData{lookup.getKey().to_string()}, + seqno); } + return MagmaScanResult::Next(); } + } + // system collections aren't in cache so we can skip that lookup + if (!lookup.getKey().getDocKey().isInSystemCollection()) { ctx.getCacheCallback().callback(lookup); if (ctx.getCacheCallback().getStatus() == cb::engine_errc::key_already_exists) { diff --git a/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.cc b/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.cc index 1c832602bd..fa3be29c25 100644 --- a/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.cc +++ b/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.cc @@ -998,6 +998,10 @@ rocksdb::Slice RocksDBKVStore::getSeqnoSlice(const int64_t* seqno) const { return rocksdb::Slice(reinterpret_cast(seqno), sizeof(*seqno)); } +rocksdb::Slice RocksDBKVStore::getSeqnoSlice(const uint64_t* seqno) const { + return getSeqnoSlice(reinterpret_cast(seqno)); +} + int64_t RocksDBKVStore::getNumericSeqno( const rocksdb::Slice& seqnoSlice) const { assert(seqnoSlice.size() == sizeof(int64_t)); diff --git a/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.h b/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.h index 6c2a2dbe73..5a02ed7b8b 100644 --- a/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.h +++ b/engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.h @@ -434,6 +434,7 @@ class RocksDBKVStore : public KVStore { rocksdb::Slice getKeySlice(const DiskDocKey& key) const; rocksdb::Slice getSeqnoSlice(const int64_t* seqno) const; + rocksdb::Slice getSeqnoSlice(const uint64_t* seqno) const; int64_t getNumericSeqno(const rocksdb::Slice& seqnoSlice) const; std::unique_ptr makeItem(Vbid vb, diff --git a/engines/ep/src/stored-value.cc b/engines/ep/src/stored-value.cc index 439d9560de..49a290bbf1 100644 --- a/engines/ep/src/stored-value.cc +++ b/engines/ep/src/stored-value.cc @@ -480,7 +480,8 @@ static std::string getSystemEventsValueFromStoredValue(const StoredValue& sv) { std::string_view itemValue{sv.getValue()->getData(), sv.getValue()->getSize()}; - if (systemEventType == SystemEvent::Scope) { + switch (systemEventType) { + case SystemEvent::Scope: { if (sv.isDeleted()) { auto eventData = Manifest::getDropScopeEventData(itemValue); return to_string(eventData); @@ -488,7 +489,10 @@ static std::string getSystemEventsValueFromStoredValue(const StoredValue& sv) { auto eventData = Manifest::getCreateScopeEventData(itemValue); return to_string(eventData); } - } else if (systemEventType == SystemEvent::Collection) { + break; + } + case SystemEvent::Collection: + case SystemEvent::ModifyCollection: { if (sv.isDeleted()) { auto eventData = Manifest::getDropEventData(itemValue); return to_string(eventData); @@ -496,7 +500,10 @@ static std::string getSystemEventsValueFromStoredValue(const StoredValue& sv) { auto eventData = Manifest::getCreateEventData(itemValue); return to_string(eventData); } + break; } + } + throw std::invalid_argument( "getSystemEventsValueFromStoredValue(): StoredValue must be a " "SystemEvent for a collection or scope"); diff --git a/engines/ep/src/systemevent_factory.cc b/engines/ep/src/systemevent_factory.cc index 48423d09dc..94948737df 100644 --- a/engines/ep/src/systemevent_factory.cc +++ b/engines/ep/src/systemevent_factory.cc @@ -95,10 +95,12 @@ SystemEventFactory::makeCollectionEventKeyPairForRangeScan(CollectionID cid) { CollectionID SystemEventFactory::getCollectionIDFromKey(const DocKey& key) { // Input key is made up of a sequence of prefixes as per makeCollectionEvent - // or makeScopeEvent + // or makeModifyCollectionEvent // This function skips (1), checks (2) and returns 3 auto se = getSystemEventType(key); - Expects(se.first == SystemEvent::Collection); // expected Collection + // expected Collection/ModifyCollection event + Expects(se.first == SystemEvent::Collection || + se.first == SystemEvent::ModifyCollection); return cb::mcbp::unsigned_leb128::decode(se.second).first; } @@ -134,6 +136,15 @@ std::pair SystemEventFactory::getTypeAndID( return {type, cb::mcbp::unsigned_leb128::decode(buffer).first}; } +std::optional SystemEventFactory::isModifyCollection( + const DocKey& key) { + auto [event, id] = SystemEventFactory::getTypeAndID(key); + if (event != SystemEvent::ModifyCollection) { + return {}; + } + return CollectionID(id); +} + std::unique_ptr SystemEventProducerMessage::make( uint32_t opaque, queued_item& item, cb::mcbp::DcpStreamId sid) { // Always ensure decompressed as we are about to use the value diff --git a/engines/ep/src/systemevent_factory.h b/engines/ep/src/systemevent_factory.h index 1e2eab4c35..b591d4df4d 100644 --- a/engines/ep/src/systemevent_factory.h +++ b/engines/ep/src/systemevent_factory.h @@ -118,6 +118,13 @@ class SystemEventFactory { */ static std::pair getTypeAndID(const DocKey& key); + /** + * Given a key which the caller knows isSystemCollection return an optional + * which stores the ID of the modified collection iff the key represent a + * ModifyCollection event. + */ + static std::optional isModifyCollection(const DocKey& key); + private: /** * Make an Item representing the SystemEvent diff --git a/engines/ep/tests/module_tests/checkpoint_test.cc b/engines/ep/tests/module_tests/checkpoint_test.cc index b92fcec0f6..c5c893cfb3 100644 --- a/engines/ep/tests/module_tests/checkpoint_test.cc +++ b/engines/ep/tests/module_tests/checkpoint_test.cc @@ -3055,6 +3055,9 @@ TEST_P(CheckpointTest, CheckpointItemToString) { auto event = SystemEventFactory::makeCollectionEvent(CollectionID(99), {}, {}); EXPECT_EQ("cid:0x1:0x0:0x63:_collection", event->getKey().to_string()); + event = SystemEventFactory::makeModifyCollectionEvent( + CollectionID(999), {}, {}); + EXPECT_EQ("cid:0x1:0x2:0x3e7:_collection", event->getKey().to_string()); event = SystemEventFactory::makeScopeEvent(ScopeID(99), {}, {}); EXPECT_EQ("cid:0x1:0x1:0x63:_scope", event->getKey().to_string()); } diff --git a/engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc b/engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc index 56c935360f..276b8f37ec 100644 --- a/engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc +++ b/engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc @@ -4475,6 +4475,38 @@ TEST_P(CollectionsDcpPersistentOnly, ModifyCollection) { vb1->lockCollections().getCanDeduplicate(fruit)); EXPECT_EQ(CanDeduplicate::Yes, vb1->lockCollections().getCanDeduplicate(vegetable)); + + // Final stage of the test - drop one of the modified collections and check + // backfill does not play back the modify + cm.remove(CollectionEntry::vegetable); + setCollections(cookie, cm); + flush_vbucket_to_disk(vbid, 1); + + vb0.reset(); + vb1.reset(); + resetEngineAndWarmup(); + + createDcpObjects({{nullptr, 0}}); + notifyAndStepToCheckpoint(ClientOpcode::DcpSnapshotMarker, false); + + // Verify that modify vegetable is not transmitted + producer->stepAndExpect(*producers, ClientOpcode::DcpSystemEvent); + EXPECT_EQ(producers->last_system_event, id::CreateCollection); + EXPECT_EQ(producers->last_collection_id, fruit.getId()); + + producer->stepAndExpect(*producers, ClientOpcode::DcpSystemEvent); + EXPECT_EQ(producers->last_system_event, id::ModifyCollection); + EXPECT_EQ(producers->last_collection_id, fruit.getId()); + + producer->stepAndExpect(*producers, ClientOpcode::DcpSystemEvent); + EXPECT_EQ(producers->last_system_event, id::DeleteCollection); + EXPECT_EQ(producers->last_collection_id, vegetable.getId()); + + // Reacquire replica and check the backfill events + vb1 = store->getVBucket(replicaVB); + // replica received modified state + EXPECT_EQ(CanDeduplicate::No, + vb1->lockCollections().getCanDeduplicate(fruit)); } // Test that if the flatBuffesrSystemEventsEnabled==false a modification event diff --git a/engines/ep/tests/module_tests/collections/evp_store_collections_eraser_test.cc b/engines/ep/tests/module_tests/collections/evp_store_collections_eraser_test.cc index 4cddbe985b..10f18cd1e4 100644 --- a/engines/ep/tests/module_tests/collections/evp_store_collections_eraser_test.cc +++ b/engines/ep/tests/module_tests/collections/evp_store_collections_eraser_test.cc @@ -2208,6 +2208,70 @@ TEST_P(CollectionsEraserTest, MB_50747_delete_and_drop) { EXPECT_EQ(0, vb->getNumItems()); } +TEST_P(CollectionsEraserTest, drop_after_modify) { + // add two collections + CollectionsManifest cm(CollectionEntry::dairy); + vb->updateFromManifest(folly::SharedMutex::ReadHolder(vb->getStateLock()), + makeManifest(cm.add(CollectionEntry::fruit))); + + flushVBucketToDiskIfPersistent(vbid, 2 /* 2 x system */); + + // add some items + store_item( + vbid, StoredDocKey{"dairy:milk", CollectionEntry::dairy}, "nice"); + store_item(vbid, + StoredDocKey{"dairy:butter", CollectionEntry::dairy}, + "lovely"); + store_item( + vbid, StoredDocKey{"fruit:apple", CollectionEntry::fruit}, "nice"); + store_item(vbid, + StoredDocKey{"fruit:apricot", CollectionEntry::fruit}, + "lovely"); + + flushVBucketToDiskIfPersistent(vbid, 4); + + EXPECT_EQ(4, vb->getNumItems()); + + // Modify history setting of dairy false->true + vb->updateFromManifest( + folly::SharedMutex::ReadHolder(vb->getStateLock()), + makeManifest(cm.update( + CollectionEntry::fruit, cb::NoExpiryLimit, true))); + store_item( + vbid, StoredDocKey{"fruit:apple", CollectionEntry::fruit}, "nice"); + + flushVBucketToDiskIfPersistent(vbid, 2); + + // delete the collections + vb->updateFromManifest( + folly::SharedMutex::ReadHolder(vb->getStateLock()), + makeManifest(cm.remove(CollectionEntry::dairy) + .remove(CollectionEntry::fruit))); + + EXPECT_FALSE(vb->lockCollections().exists(CollectionEntry::dairy)); + EXPECT_FALSE(vb->lockCollections().exists(CollectionEntry::fruit)); + + flushVBucketToDiskIfPersistent(vbid, 2 /* 2 x system */); + + if (!isPersistent()) { + // 2x create collection, 1x modify + EXPECT_EQ(3, vb->ht.getNumSystemItems()); + } + + runCollectionsEraser(vbid); + + EXPECT_EQ(0, vb->getNumItems()); + + EXPECT_FALSE(vb->lockCollections().exists(CollectionEntry::dairy)); + EXPECT_FALSE(vb->lockCollections().exists(CollectionEntry::fruit)); + + if (!isPersistent()) { + // 2 system events remain (tombstones of dairy/fruit). + // modify was purged + EXPECT_EQ(2, vb->ht.getNumSystemItems()); + } +} + // Test cases which run for persistent and ephemeral buckets INSTANTIATE_TEST_SUITE_P(CollectionsEraserTests, CollectionsEraserTest, diff --git a/engines/ep/tests/module_tests/evp_store_single_threaded_test.cc b/engines/ep/tests/module_tests/evp_store_single_threaded_test.cc index 2f9bd53ac6..b016a09835 100644 --- a/engines/ep/tests/module_tests/evp_store_single_threaded_test.cc +++ b/engines/ep/tests/module_tests/evp_store_single_threaded_test.cc @@ -438,6 +438,8 @@ void SingleThreadedKVBucketTest::runCompaction(Vbid id, void SingleThreadedKVBucketTest::scheduleAndRunCollectionsEraser( Vbid id, bool expectSuccess) { if (isPersistent()) { + auto failures = engine->getEpStats().compactionFailed; + runCompaction(id, 0, false); if (expectSuccess) { @@ -456,6 +458,9 @@ void SingleThreadedKVBucketTest::scheduleAndRunCollectionsEraser( // are still present on disk for PiTR tests. EXPECT_TRUE(dropped.empty()); } + EXPECT_EQ(failures, engine->getEpStats().compactionFailed); + } else { + EXPECT_LT(failures, engine->getEpStats().compactionFailed); } } else { auto* bucket = dynamic_cast(store); diff --git a/utilities/dockey.cc b/utilities/dockey.cc index 0ae4eeabfc..fe377d66c2 100644 --- a/utilities/dockey.cc +++ b/utilities/dockey.cc @@ -134,8 +134,11 @@ std::string DocKey::to_string() const { auto [cidOrSid, keySuffix] = cb::mcbp::unsigned_leb128::decode( {keyWithoutEvent.data(), keyWithoutEvent.size()}); - if (systemEventID == uint32_t(SystemEvent::Collection) || - systemEventID == uint32_t(SystemEvent::Scope)) { + + switch (SystemEvent(systemEventID)) { + case SystemEvent::Collection: + case SystemEvent::Scope: + case SystemEvent::ModifyCollection: // Get the string view to the remaining string part of the key remainingKey = {reinterpret_cast(keySuffix.data()), keySuffix.size()};