Keep a document meta store read guard together with bitvector posting list in search cache for imported attributes.

This is to ensure that no lids that are cached in the bitvector are re-used until the guard is released.

10 files changed, 68 insertions, 12 deletions

diff --git a/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp b/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp
index 1f738a209df..0072f83bc54 100644
--- a/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp
+++ b/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp
@@ -3,6 +3,7 @@
 #include <vespa/vespalib/testkit/test_kit.h>
 #include <vespa/searchlib/attribute/bitvector_search_cache.h>
 #include <vespa/searchlib/common/bitvector.h>
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 
 using namespace search;
 using namespace search::attribute;
@@ -13,7 +14,7 @@ using Entry = BitVectorSearchCache::Entry;
 Entry::SP
 makeEntry()
 {
-    return std::make_shared<Entry>(BitVector::create(5), 10);
+    return std::make_shared<Entry>(IDocumentMetaStoreContext::IReadGuard::UP(), BitVector::create(5), 10);
 }
 
 struct Fixture {
diff --git a/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp b/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
index f6ab6bb50bf..4ab0ed878cf 100644
--- a/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
+++ b/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
@@ -370,7 +370,7 @@ makeSearchCacheEntry(const std::vector<uint32_t> docIds, uint32_t docIdLimit)
     for (uint32_t docId : docIds) {
         bitVector->setBit(docId);
     }
-    return std::make_shared<BitVectorSearchCache::Entry>(bitVector, docIdLimit);
+    return std::make_shared<BitVectorSearchCache::Entry>(IDocumentMetaStoreContext::IReadGuard::UP(), bitVector, docIdLimit);
 }
 
 TEST_F("Bit vector from search cache is used if found", SearchCacheFixture)
@@ -382,6 +382,7 @@ TEST_F("Bit vector from search cache is used if found", SearchCacheFixture)
     TermFieldMatchData match;
     auto iter = f.create_strict_iterator(*ctx, match);
     TEST_DO(f.assertSearch({2, 6}, *iter)); // Note: would be {3, 5} if cache was not used
+    EXPECT_EQUAL(0u, f.document_meta_store->get_read_guard_cnt);
 }
 
 void
@@ -405,6 +406,7 @@ TEST_F("Entry is inserted into search cache if bit vector posting list is used",
     auto cacheEntry = f.imported_attr->getSearchCache()->find("5678");
     EXPECT_EQUAL(cacheEntry->docIdLimit, f.imported_attr->getNumDocs());
     TEST_DO(assertBitVector({3, 5}, *cacheEntry->bitVector));
+    EXPECT_EQUAL(1u, f.document_meta_store->get_read_guard_cnt);
 }
 
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h b/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h
index 73fb95e1752..a889120f8df 100644
--- a/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h
+++ b/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h
@@ -2,6 +2,7 @@
 
 #pragma once
 
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 #include <vespa/vespalib/stllike/hash_map.h>
 #include <vespa/vespalib/stllike/string.h>
 #include <memory>
@@ -21,13 +22,17 @@ namespace attribute {
 class BitVectorSearchCache {
 public:
     using BitVectorSP = std::shared_ptr<BitVector>;
+    using ReadGuardUP = IDocumentMetaStoreContext::IReadGuard::UP;
 
     struct Entry {
         using SP = std::shared_ptr<Entry>;
+        // We need to keep a document meta store read guard to ensure that no lids that are cached
+        // in the bit vector are re-used until the guard is released.
+        ReadGuardUP dmsReadGuard;
         BitVectorSP bitVector;
         uint32_t docIdLimit;
-        Entry(BitVectorSP bitVector_, uint32_t docIdLimit_)
-            : bitVector(std::move(bitVector_)), docIdLimit(docIdLimit_) {}
+        Entry(ReadGuardUP dmsReadGuard_, BitVectorSP bitVector_, uint32_t docIdLimit_)
+            : dmsReadGuard(std::move(dmsReadGuard_)), bitVector(std::move(bitVector_)), docIdLimit(docIdLimit_) {}
     };
 
 private:
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
index 9efac9ce541..a085e21d6ae 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
@@ -14,10 +14,12 @@ ImportedAttributeVector::ImportedAttributeVector(
             vespalib::stringref name,
             std::shared_ptr<ReferenceAttribute> reference_attribute,
             std::shared_ptr<AttributeVector> target_attribute,
+            std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
             bool use_search_cache)
     : _name(name),
       _reference_attribute(std::move(reference_attribute)),
       _target_attribute(std::move(target_attribute)),
+      _document_meta_store(std::move(document_meta_store)),
       _search_cache(use_search_cache ? std::make_shared<BitVectorSearchCache>() :
                     std::shared_ptr<BitVectorSearchCache>())
 {
@@ -26,10 +28,12 @@ ImportedAttributeVector::ImportedAttributeVector(
 ImportedAttributeVector::ImportedAttributeVector(vespalib::stringref name,
                                                  std::shared_ptr<ReferenceAttribute> reference_attribute,
                                                  std::shared_ptr<AttributeVector> target_attribute,
+                                                 std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                                                  std::shared_ptr<BitVectorSearchCache> search_cache)
     : _name(name),
       _reference_attribute(std::move(reference_attribute)),
       _target_attribute(std::move(target_attribute)),
+      _document_meta_store(std::move(document_meta_store)),
       _search_cache(std::move(search_cache))
 {
 }
@@ -41,7 +45,7 @@ std::unique_ptr<ImportedAttributeVector>
 ImportedAttributeVector::makeReadGuard(bool stableEnumGuard) const
 {
     return std::make_unique<ImportedAttributeVectorReadGuard>
-        (getName(), getReferenceAttribute(), getTargetAttribute(), getSearchCache(), stableEnumGuard);
+        (getName(), getReferenceAttribute(), getTargetAttribute(), getDocumentMetaStore(), getSearchCache(), stableEnumGuard);
 }
 
 const vespalib::string& search::attribute::ImportedAttributeVector::getName() const {
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
index c94eda3d4db..7d272c42d9e 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
@@ -11,6 +11,7 @@ namespace search {
 
 class AttributeGuard;
 class AttributeEnumGuard;
+class IDocumentMetaStoreContext;
 
 namespace attribute {
 
@@ -32,10 +33,12 @@ public:
     ImportedAttributeVector(vespalib::stringref name,
                             std::shared_ptr<ReferenceAttribute> reference_attribute,
                             std::shared_ptr<AttributeVector> target_attribute,
+                            std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                             bool use_search_cache);
     ImportedAttributeVector(vespalib::stringref name,
                             std::shared_ptr<ReferenceAttribute> reference_attribute,
                             std::shared_ptr<AttributeVector> target_attribute,
+                            std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                             std::shared_ptr<BitVectorSearchCache> search_cache);
     ~ImportedAttributeVector();
 
@@ -72,6 +75,9 @@ public:
     const std::shared_ptr<AttributeVector>& getTargetAttribute() const noexcept {
         return _target_attribute;
     }
+    const std::shared_ptr<IDocumentMetaStoreContext> &getDocumentMetaStore() const {
+        return _document_meta_store;
+    }
     const std::shared_ptr<BitVectorSearchCache> &getSearchCache() const {
         return _search_cache;
     }
@@ -88,10 +94,11 @@ protected:
                                       const common::BlobConverter * bc) const override;
 
 
-    vespalib::string                      _name;
-    std::shared_ptr<ReferenceAttribute>   _reference_attribute;
-    std::shared_ptr<AttributeVector>      _target_attribute;
-    std::shared_ptr<BitVectorSearchCache> _search_cache;
+    vespalib::string                           _name;
+    std::shared_ptr<ReferenceAttribute>        _reference_attribute;
+    std::shared_ptr<AttributeVector>           _target_attribute;
+    std::shared_ptr<IDocumentMetaStoreContext> _document_meta_store;
+    std::shared_ptr<BitVectorSearchCache>      _search_cache;
 };
 
 } // attribute
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
index 4eedb6ce915..2860769235d 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
@@ -9,9 +9,11 @@ ImportedAttributeVectorReadGuard::ImportedAttributeVectorReadGuard(
         vespalib::stringref name,
         std::shared_ptr<ReferenceAttribute> reference_attribute,
         std::shared_ptr<AttributeVector> target_attribute,
+        std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
         std::shared_ptr<BitVectorSearchCache> search_cache,
         bool stableEnumGuard)
-    : ImportedAttributeVector(name, std::move(reference_attribute), std::move(target_attribute), std::move(search_cache)),
+    : ImportedAttributeVector(name, std::move(reference_attribute), std::move(target_attribute),
+                              std::move(document_meta_store), std::move(search_cache)),
       _referencedLids(),
       _reference_attribute_guard(_reference_attribute),
       _target_attribute_guard(stableEnumGuard ? std::shared_ptr<AttributeVector>() : _target_attribute),
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
index 7b226d2556a..1937953df66 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
@@ -31,6 +31,7 @@ public:
     ImportedAttributeVectorReadGuard(vespalib::stringref name,
                                      std::shared_ptr<ReferenceAttribute> reference_attribute,
                                      std::shared_ptr<AttributeVector> target_attribute,
+                                     std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                                      std::shared_ptr<BitVectorSearchCache> search_cache,
                                      bool stableEnumGuard);
     ~ImportedAttributeVectorReadGuard();
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp b/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp
index d2dc52ec2c7..a21ecacccdb 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp
@@ -29,6 +29,8 @@ ImportedSearchContext::ImportedSearchContext(
       _useSearchCache(_imported_attribute.getSearchCache().get() != nullptr),
       _searchCacheLookup((_useSearchCache ? _imported_attribute.getSearchCache()->find(_queryTerm) :
                           std::shared_ptr<BitVectorSearchCache::Entry>())),
+      _dmsReadGuard((_useSearchCache && !_searchCacheLookup) ? imported_attribute.getDocumentMetaStore()->getReadGuard() :
+                    std::unique_ptr<IDocumentMetaStoreContext::IReadGuard>()),
       _reference_attribute(*_imported_attribute.getReferenceAttribute()),
       _target_attribute(*_imported_attribute.getTargetAttribute()),
       _target_search_context(_target_attribute.getSearch(std::move(term), params)),
@@ -36,6 +38,7 @@ ImportedSearchContext::ImportedSearchContext(
       _merger(_reference_attribute.getCommittedDocIdLimit()),
       _fetchPostingsDone(false)
 {
+
 }
 
 ImportedSearchContext::~ImportedSearchContext() {
@@ -215,7 +218,8 @@ void
 ImportedSearchContext::considerAddSearchCacheEntry()
 {
     if (_useSearchCache && _merger.hasBitVector()) {
-        auto cacheEntry = std::make_shared<BitVectorSearchCache::Entry>(_merger.getBitVectorSP(), _merger.getDocIdLimit());
+        assert(_dmsReadGuard);
+        auto cacheEntry = std::make_shared<BitVectorSearchCache::Entry>(std::move(_dmsReadGuard), _merger.getBitVectorSP(), _merger.getDocIdLimit());
         _imported_attribute.getSearchCache()->insert(_queryTerm, std::move(cacheEntry));
     }
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_search_context.h b/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
index 0f6f8125cf8..0ddca8c9fe5 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
@@ -6,6 +6,7 @@
 #include "bitvector_search_cache.h"
 #include <vespa/searchcommon/attribute/i_search_context.h>
 #include <vespa/searchlib/attribute/posting_list_merger.h>
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 #include <vespa/vespalib/util/arrayref.h>
 
 namespace search::fef { class TermFieldMatchData; }
@@ -30,6 +31,7 @@ class ImportedSearchContext : public ISearchContext {
     vespalib::string                                _queryTerm;
     bool                                            _useSearchCache;
     BitVectorSearchCache::Entry::SP                 _searchCacheLookup;
+    IDocumentMetaStoreContext::IReadGuard::UP       _dmsReadGuard;
     const ReferenceAttribute&                       _reference_attribute;
     const AttributeVector&                          _target_attribute;
     std::unique_ptr<AttributeVector::SearchContext> _target_search_context;
diff --git a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
index 3620a1df725..f9e3de79144 100644
--- a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
+++ b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
@@ -12,6 +12,7 @@
 #include <vespa/searchlib/attribute/integerbase.h>
 #include <vespa/searchlib/attribute/not_implemented_attribute.h>
 #include <vespa/searchlib/attribute/stringbase.h>
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 #include <vespa/searchlib/query/queryterm.h>
 #include <vespa/searchcommon/attribute/attributecontent.h>
 #include <vespa/vespalib/testkit/testapp.h>
@@ -23,6 +24,27 @@
 #include <vector>
 
 namespace search {
+
+struct MockDocumentMetaStoreContext : public IDocumentMetaStoreContext {
+
+    struct MockReadGuard : public IDocumentMetaStoreContext::IReadGuard {
+        virtual const search::IDocumentMetaStore &get() const override {
+            search::IDocumentMetaStore *nullStore = nullptr;
+            return static_cast<search::IDocumentMetaStore &>(*nullStore);
+        }
+    };
+
+    mutable size_t get_read_guard_cnt;
+
+    using SP = std::shared_ptr<MockDocumentMetaStoreContext>;
+    MockDocumentMetaStoreContext() : get_read_guard_cnt(0) {}
+
+    virtual IReadGuard::UP getReadGuard() const override {
+        ++get_read_guard_cnt;
+        return std::make_unique<MockReadGuard>();
+    }
+};
+
 namespace attribute {
 
 using document::DocumentId;
@@ -39,6 +61,10 @@ std::shared_ptr<ReferenceAttribute> create_reference_attribute(vespalib::stringr
     return std::make_shared<ReferenceAttribute>(name, Config(BasicType::REFERENCE));
 }
 
+MockDocumentMetaStoreContext::SP create_document_meta_store() {
+    return std::make_shared<MockDocumentMetaStoreContext>();
+}
+
 enum class FastSearchConfig {
     ExplicitlyEnabled,
     Default
@@ -105,6 +131,7 @@ struct ImportedAttributeFixture {
     bool use_search_cache;
     std::shared_ptr<AttributeVector> target_attr;
     std::shared_ptr<ReferenceAttribute> reference_attr;
+    MockDocumentMetaStoreContext::SP document_meta_store;
     std::shared_ptr<ImportedAttributeVector> imported_attr;
     std::shared_ptr<MockGidToLidMapperFactory> mapper_factory;
 
@@ -130,7 +157,7 @@ struct ImportedAttributeFixture {
 
     std::shared_ptr<ImportedAttributeVector>
     create_attribute_vector_from_members(vespalib::stringref name = default_imported_attr_name()) {
-        return std::make_shared<ImportedAttributeVector>(name, reference_attr, target_attr, use_search_cache);
+        return std::make_shared<ImportedAttributeVector>(name, reference_attr, target_attr, document_meta_store, use_search_cache);
     }
 
     template<typename AttrVecType>
@@ -225,6 +252,7 @@ ImportedAttributeFixture::ImportedAttributeFixture(bool use_search_cache_)
         : use_search_cache(use_search_cache_),
           target_attr(create_single_attribute<IntegerAttribute>(BasicType::INT32)),
           reference_attr(create_reference_attribute()),
+          document_meta_store(create_document_meta_store()),
           imported_attr(create_attribute_vector_from_members()),
           mapper_factory(std::make_shared<MockGidToLidMapperFactory>()) {
     reference_attr->setGidToLidMapperFactory(mapper_factory);