Merge pull request #30291 from vespa-engine/toregge/add-test-for-predicate-index-saver-observing-a-snapshot

Test that predicate index saver protected by a generation guard

2 files changed, 56 insertions, 4 deletions

diff --git a/searchlib/src/tests/predicate/predicate_index_test.cpp b/searchlib/src/tests/predicate/predicate_index_test.cpp
index 6351807f4fc..31098feddbc 100644
--- a/searchlib/src/tests/predicate/predicate_index_test.cpp
+++ b/searchlib/src/tests/predicate/predicate_index_test.cpp
@@ -20,6 +20,7 @@ using namespace search::predicate;
 using std::make_pair;
 using std::pair;
 using std::vector;
+using vespalib::DataBuffer;
 
 namespace {
 
@@ -34,7 +35,7 @@ DummyDocIdLimitProvider dummy_provider;
 SimpleIndexConfig simple_index_config;
 
 void
-save_predicate_index(PredicateIndex& index, vespalib::DataBuffer& buffer)
+save_predicate_index(PredicateIndex& index, DataBuffer& buffer)
 {
     index.commit();
     DataBufferWriter writer(buffer);
@@ -42,6 +43,43 @@ save_predicate_index(PredicateIndex& index, vespalib::DataBuffer& buffer)
     writer.flush();
 }
 
+class GuardedSaver {
+    vespalib::GenerationHandler::Guard _guard;
+    std::unique_ptr<ISaver>            _saver;
+public:
+    GuardedSaver(vespalib::GenerationHandler::Guard guard, std::unique_ptr<ISaver> saver)
+        : _guard(std::move(guard)),
+          _saver(std::move(saver))
+    {
+    }
+    ~GuardedSaver();
+    DataBuffer save() const {
+        DataBuffer buffer;
+        DataBufferWriter writer(buffer);
+        _saver->save(writer);
+        writer.flush();
+        return buffer;
+    }
+};
+
+GuardedSaver::~GuardedSaver() = default;
+
+GuardedSaver
+make_guarded_saver(PredicateIndex& index)
+{
+    index.commit();
+    auto guard = generation_handler.takeGuard();
+    auto saver = index.make_saver();
+    return { std::move(guard), std::move(saver) };
+}
+
+bool
+equal_buffers(const DataBuffer& lhs, const DataBuffer& rhs)
+{
+    return (lhs.getDataLen() ==  rhs.getDataLen()) &&
+        (memcmp(lhs.getData(), rhs.getData(), lhs.getDataLen()) == 0);
+}
+
 TEST("require that PredicateIndex can index empty documents") {
     PredicateIndex index(generation_holder, dummy_provider, simple_index_config, 10);
     EXPECT_EQUAL(0u, index.getZeroConstraintDocs().size());
@@ -301,7 +339,7 @@ TEST("require that PredicateIndex can be (de)serialized") {
     }
     index.commit();
 
-    vespalib::DataBuffer buffer;
+    DataBuffer buffer;
     save_predicate_index(index, buffer);
     uint32_t doc_id_limit;
     DocIdLimitFinder finder(doc_id_limit);
@@ -345,7 +383,7 @@ TEST("require that DocumentFeaturesStore is restored on deserialization") {
     PredicateIndex index(generation_holder, dummy_provider, simple_index_config, 10);
     EXPECT_FALSE(index.getIntervalIndex().lookup(hash).valid());
     indexFeature(index, doc_id, min_feature, {{hash, interval}}, {{hash2, bounds}});
-    vespalib::DataBuffer buffer;
+    DataBuffer buffer;
     save_predicate_index(index, buffer);
     uint32_t doc_id_limit;
     DocIdLimitFinder finder(doc_id_limit);
@@ -379,6 +417,20 @@ TEST("require that hold lists are attempted emptied on destruction") {
     }
     // No assert on index destruction.
 }
+
+TEST("require that predicate index saver protected by a generation guard observes a snapshot of the predicate index")
+{
+    PredicateIndex index(generation_holder, dummy_provider, simple_index_config, 10);
+    indexFeature(index, doc_id, min_feature, {{hash, interval}}, {{hash2, bounds}});
+    auto saver1 = make_guarded_saver(index);
+    auto buf1 = saver1.save();
+    index.removeDocument(doc_id);
+    index.commit();
+    auto saver2 = make_guarded_saver(index);
+    EXPECT_TRUE(equal_buffers(buf1, saver1.save()));
+    EXPECT_FALSE(equal_buffers(buf1, saver2.save()));
+}
+
 }  // namespace
 
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchlib/src/vespa/searchlib/predicate/document_features_store_saver.h b/searchlib/src/vespa/searchlib/predicate/document_features_store_saver.h
index 289b1fc076f..0fd927a7262 100644
--- a/searchlib/src/vespa/searchlib/predicate/document_features_store_saver.h
+++ b/searchlib/src/vespa/searchlib/predicate/document_features_store_saver.h
@@ -19,7 +19,7 @@ class DocumentFeaturesStoreSaver : public ISaver {
     using RangesStore = DocumentFeaturesStore::RangesStore;
     using WordStore = DocumentFeaturesStore::WordStore;
 
-    const RefsVector&     _refs;  // TODO: Use copy when saving in flush thread
+    const RefsVector      _refs;
     const FeaturesStore&  _features;
     const RangesStore&    _ranges;
     const WordStore&      _word_store;