MB-59452: Don't use xattrs for collection related metadata

The addition of modify collection in 7.2 required some extra work to
keep support for "legacy" DCP users, that is to not expose the seqno
of the modify event through various APIs (e.g. getAllSeqnos).

The orginal change did this by stashing this one-off seqno into an
xattr blob attached to the value of the modify event. Thus only
the modify of the default collection ever stored this extra data.

MB-59452 reminded us that xattrs can introduce a performance cost and
the modify of the default collection should not be the trigger for
a user to pay that cost.

Fix by moving the extra metadata into the flatbuffer data so no xattr
values are generated by KV itself.

1) The modify collection "write" path now changes to always stash the
special seqno inside the flatbuffers.

2) The warmup path checks for xattr and can load both encodings.

Note there is no explicit "sanitise" path. DCP never transmitted the
xattr part of the modify and the special seqno was always recalculated
by the recipient. In this case a rebalance will result in the xattr
blob being removed and never stored again.

Change-Id: I33872e304f0ec53ec2ba3de8cf161546ba12ff0c
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/201296
Well-Formed: Restriction Checker
Tested-by: Jim Walker <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jimwwalker

engines/ep/src/collections/events.fbs

@@ -27,6 +27,14 @@ table Collection {
     // history: MB-54516: added in neo (7.2) branch. If this field is not part
     // of a Collection structure it is defaulted to false.
     history:bool = false;
+
+    // All fields after here are added from 7.2.4
+    //
+    // defaultCollectionMVS: MB-59452: This field exists to assist in the
+    // tracking of what is the max-visible seqno of the default collection. This
+    // field is only written to when the event is a modification of the default
+    // collection and serves no purpose outside of KV-engine.
+    defaultCollectionMVS:ulong = 0;
 }
 
 // FlatBuffers SystemEvents: This is the structure used by DropCollection

engines/ep/src/collections/vbucket_manifest.cc

@@ -968,7 +968,8 @@ std::unique_ptr<Item> Manifest::makeCollectionSystemEvent(
         std::string_view collectionName,
         const ManifestEntry& entry,
         SystemEventType type,
-        OptionalSeqno seq) {
+        OptionalSeqno seq,
+        uint64_t defaultCollectionMaxLegacyDCPSeqno) {
     flatbuffers::FlatBufferBuilder builder;
 
     switch (type) {
@@ -985,7 +986,9 @@ std::unique_ptr<Item> Manifest::makeCollectionSystemEvent(
                                               : 0,
                 builder.CreateString(collectionName.data(),
                                      collectionName.size()),
-                getHistoryFromCanDeduplicate(entry.getCanDeduplicate()));
+                getHistoryFromCanDeduplicate(entry.getCanDeduplicate()),
+                cid.isDefaultCollection() ? defaultCollectionMaxLegacyDCPSeqno
+                                          : 0);
         builder.Finish(collection);
         break;
     }
@@ -1033,12 +1036,13 @@ uint64_t Manifest::queueCollectionSystemEvent(
         vb.checkpointManager->createNewCheckpoint();
     }
 
-    auto item = makeCollectionSystemEvent(
-            getManifestUid(), cid, collectionName, entry, type, seq);
-
-    if (type == SystemEventType::Modify && cid.isDefaultCollection()) {
-        attachMaxLegacyDCPSeqno(*item);
-    }
+    auto item = makeCollectionSystemEvent(getManifestUid(),
+                                          cid,
+                                          collectionName,
+                                          entry,
+                                          type,
+                                          seq,
+                                          defaultCollectionMaxLegacyDCPSeqno);
 
     // Create and transfer Item ownership to the VBucket
     auto rv = vb.addSystemEventItem(
@@ -1069,12 +1073,13 @@ void Manifest::setDefaultCollectionLegacySeqnos(uint64_t maxCommittedSeqno,
 }
 
 // Build an XATTR pair which records the defaultCollectionMaxLegacyDCPSeqno
-void Manifest::attachMaxLegacyDCPSeqno(Item& item) const {
+void Manifest::attachMaxLegacyDCPSeqno(
+        Item& item, uint64_t seqno) {
     Expects(!mcbp::datatype::is_xattr(item.getDataType()));
     cb::xattr::Blob xattrs;
     xattrs.set(
             LegacyXattrKey,
-            fmt::format(LegacyJSONFormat, defaultCollectionMaxLegacyDCPSeqno));
+            fmt::format(LegacyJSONFormat, seqno));
 
     std::string xattrValue;
     xattrValue.reserve(xattrs.size() + item.getNBytes());
@@ -1110,16 +1115,29 @@ uint64_t Manifest::computeDefaultCollectionMaxLegacyDCPSeqno(
         // The default collections current high-seqno is the maxLegacyDCP value
         return highSeqno;
     }
+    // It cannot be greater
+    Expects(uint64_t(item.getBySeqno()) == highSeqno);
+
+    // else the modify is the highSeqno and it stores the correct legacy seqno.
+    // Retrieve the data which is either in xattr or the value
 
-    // else the modify is the highSeqno and it stores the correct legacy seqno
-    // see attachMaxLegacyDCPSeqno which is the encoder of this data.
+    uint64_t storedSeqno{0};
+    // Get the seqno from the correct part of the value.
+    if (mcbp::datatype::is_xattr(item.getDataType())) {
+        // 7.2 began by stashing in xattrs.
+        storedSeqno = getSeqnoFromXattr(item);
+    } else {
+        storedSeqno = getSeqnoFromFlatBuffer(item);
+    }
+    Expects(storedSeqno < highSeqno);
+    return storedSeqno;
+}
 
+uint64_t Manifest::getSeqnoFromXattr(const Item& item) {
     // Found it. All modify events have xattr
     Expects(mcbp::datatype::is_xattr(item.getDataType()));
     // This should be decompressed
     Expects(!mcbp::datatype::is_snappy(item.getDataType()));
-    // It cannot be greater
-    Expects(uint64_t(item.getBySeqno()) == highSeqno);
 
     // pull out the stashed seqno which the VB:Manifest needs
     cb::xattr::Blob xattr({const_cast<char*>(item.getData()), item.getNBytes()},
@@ -1135,12 +1153,18 @@ uint64_t Manifest::computeDefaultCollectionMaxLegacyDCPSeqno(
     auto stashedSeqno = std::stoull(max->get<std::string>());
 
     Expects(stashedSeqno < uint64_t(item.getBySeqno()));
-    Expects(stashedSeqno < highSeqno);
 
     // return this value to use as the max-legacy seqno
     return stashedSeqno;
 }
 
+uint64_t Manifest::getSeqnoFromFlatBuffer(const Item& item) {
+    Expects(!mcbp::datatype::is_xattr(item.getDataType()));
+    const auto& collection =
+            Collections::VB::Manifest::getCollectionFlatbuffer(item);
+    return collection.defaultCollectionMVS();
+}
+
 size_t Manifest::getSystemEventItemCount() const {
     // Every 'live' collection has 1 'live' item, except for the default
     // collection which has no creation event.

engines/ep/src/collections/vbucket_manifest.h

@@ -370,7 +370,15 @@ class Manifest {
             std::string_view collectionName,
             const ManifestEntry& entry,
             SystemEventType type,
-            OptionalSeqno seq);
+            OptionalSeqno seq,
+            uint64_t defaultCollectionMaxLegacyDCPSeqno = 0);
+
+    /**
+     * Store "seqno" into the item using an xattr. This function only exists for
+     * use by tests checking that KV can warm-up when a 7.2 modify default
+     * collection system event is on-disk. See MB-59452
+     */
+    static void attachMaxLegacyDCPSeqno(Item& item, uint64_t seqno);
 
     bool operator==(const Manifest& rhs) const;
 
@@ -1159,13 +1167,6 @@ class Manifest {
      */
     CanDeduplicate getCanDeduplicate(CollectionID cid) const;
 
-    /**
-     * Store the value of defaultCollectionMaxLegacyDCPSeqno inside the Item
-     * This function utilises xattrs so that only a modify of the default
-     * collection carries this extra data. No other event needs this data.
-     */
-    void attachMaxLegacyDCPSeqno(Item& item) const;
-
     /**
      * Function returns the correct value for defaultCollectionMaxLegacyDCPSeqno
      * by reading any modify of the default collection from KVStore and then
@@ -1180,6 +1181,22 @@ class Manifest {
                                                        Vbid vb,
                                                        KVStoreIface& kvs);
 
+    /**
+     * Retrieve from the Item the stored max visible default collection seqno
+     * that is assumed to be stored within the Item. Caller is expected to have
+     * checked that the Item is a modify of the default collection and the
+     * datatype is xattr.
+     */
+    static uint64_t getSeqnoFromXattr(const Item& item);
+
+    /**
+     * Retrieve from the Item the stored max visible default collection seqno
+     * that is assumed to be stored within the Item. Caller is expected to have
+     * checked that the Item is a modify of the default collection and the
+     * datatype is not xattr (value is thus inside the flatbuffer value.
+     */
+    static uint64_t getSeqnoFromFlatBuffer(const Item& item);
+
     /**
      * Return a string for use in throwException, returns:
      *   "VB::Manifest::<thrower>:<error>, this:<ostream *this>"

engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc

@@ -3006,6 +3006,8 @@ class CollectionsDcpPersistentOnly : public CollectionsDcpParameterizedTest {
 public:
     void resurrectionTest(bool dropAtEnd, bool updateItemPath, bool deleteItem);
     void resurrectionStatsTest(bool reproduceUnderflow, bool updateItemDropped);
+    void defaultCollectionLegacySeqnos(bool modifyWithoutXattr);
+    std::unique_ptr<Item> makeModifyWithXattr();
 };
 
 // Observed in MB-39864, the data we store in _local had a collection with
@@ -4605,7 +4607,8 @@ TEST_P(CollectionsDcpPersistentOnly, ModifyFilteredCollection) {
     EXPECT_EQ(producers->last_can_deduplicate, CanDeduplicate::Yes);
 }
 
-TEST_P(CollectionsDcpPersistentOnly, DefaultCollectionLegacySeqnos) {
+void CollectionsDcpPersistentOnly::defaultCollectionLegacySeqnos(
+        bool modifyWithoutXattr) {
     using namespace cb::mcbp;
     using namespace mcbp::systemevent;
     using namespace CollectionEntry;
@@ -4663,10 +4666,23 @@ TEST_P(CollectionsDcpPersistentOnly, DefaultCollectionLegacySeqnos) {
         validate(2, 2, 2);
     }
 
-    // Modify default collection, so only high-seqno moves
-    CollectionsManifest cm;
-    cm.update(defaultC, cb::NoExpiryLimit, true /*history*/);
-    setCollections(cookie, cm);
+    if (modifyWithoutXattr) {
+        // Modify default collection, so only high-seqno moves
+        CollectionsManifest cm;
+        cm.update(defaultC, cb::NoExpiryLimit, true /*history*/);
+        setCollections(cookie, cm);
+    } else {
+        // by-pass the manifest update path as this variant of the test needs to
+        // create an xattr system-event as per 7.2
+        EXPECT_EQ(3,
+                  store->getVBucket(vbid)->addSystemEventItem(
+                          makeModifyWithXattr(),
+                          {},
+                          CollectionID::Default,
+                          store->getVBucket(vbid)->getManifest().wlock(),
+                          [](uint64_t) {}));
+    }
+
     {
         SCOPED_TRACE("Modify collection @ seqno 3");
         validate(2, 2, 3);
@@ -4730,6 +4746,44 @@ TEST_P(CollectionsDcpPersistentOnly, DefaultCollectionLegacySeqnos) {
     }
 }
 
+TEST_P(CollectionsDcpPersistentOnly, DefaultCollectionLegacySeqnos) {
+    defaultCollectionLegacySeqnos(true);
+}
+
+std::unique_ptr<Item> CollectionsDcpPersistentOnly::makeModifyWithXattr() {
+    // Here we have to manually inject the 7.2 modify event. Cannot use a real
+    // manifest driven event as it will be using flatbuffers.
+    // Build the system event value, enable history
+    flatbuffers::FlatBufferBuilder builder;
+    std::string name{"_default"};
+    auto collection = Collections::VB::CreateCollection(
+            builder,
+            1, // uid
+            uint32_t(ScopeID::Default),
+            uint32_t(CollectionID::Default),
+            false, // no maxTTL
+            0, // no maxTTL
+            builder.CreateString(name.data(), name.size()),
+            true, // history enabled
+            0);
+    builder.Finish(collection);
+    // Create the Item (so it has the system event key etc...)
+    auto event = SystemEventFactory::makeModifyCollectionEvent(
+            CollectionID::Default,
+            {builder.GetBufferPointer(), builder.GetSize()},
+            {});
+
+    // Use the code which remains in VB::Manifest that glues a seqno into the
+    // event using xattr. Here we add 2 as the legacy seqno
+    Collections::VB::Manifest::attachMaxLegacyDCPSeqno(*event, 2);
+
+    return event;
+}
+
+TEST_P(CollectionsDcpPersistentOnly, DefaultCollectionLegacySeqnoFromXattr) {
+    defaultCollectionLegacySeqnos(false);
+}
+
 TEST_P(CollectionsDcpPersistentOnly, getRangeCountingSystemEvents) {
     using namespace CollectionEntry;