Add minimal snapshot read guard interface to bucket DB

Only exposes enough functionality to be used for Get operations for now.
Enable free-lists for underlying replica `ArrayStore`.
Legacy `MapBucketDatabase` read guard is _not_ thread safe, as it will never
be used for non-blocking reads.

6 files changed, 111 insertions, 18 deletions

diff --git a/storage/src/tests/distributor/bucketdatabasetest.cpp b/storage/src/tests/distributor/bucketdatabasetest.cpp
index 6225865b153..43b0e041f03 100644
--- a/storage/src/tests/distributor/bucketdatabasetest.cpp
+++ b/storage/src/tests/distributor/bucketdatabasetest.cpp
@@ -170,8 +170,15 @@ BucketDatabaseTest::doFindParents(const std::vector<document::BucketId>& ids,
     }
 
     std::vector<BucketDatabase::Entry> entries;
+    // TODO remove in favor of only read guard once legacy DB usage has been ported over
     db().getParents(searchId, entries);
 
+    std::vector<BucketDatabase::Entry> checked_entries;
+    db().acquire_read_guard()->find_parents_and_self(searchId, checked_entries);
+    if(entries != checked_entries) {
+        return "Mismatch between results from getParents() and ReadGuard!";
+    }
+
     std::ostringstream ost;
     for (uint32_t i = 0; i < ids.size(); ++i) {
         if (std::find(entries.begin(), entries.end(),
diff --git a/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp b/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp
index ef198cb1d3f..a901bbdd96b 100644
--- a/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp
+++ b/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp
@@ -42,14 +42,18 @@ using document::BucketId;
 
 BTreeBucketDatabase::BTreeBucketDatabase()
     : _tree(),
-      _store(ReplicaStore::optimizedConfigForHugePage(1023, vespalib::alloc::MemoryAllocator::HUGEPAGE_SIZE,
-                                                      4 * 1024, 8 * 1024, 0.2)),
+      _store(make_default_array_store_config()),
       _generation_handler()
 {
 }
 
 BTreeBucketDatabase::~BTreeBucketDatabase() = default;
 
+search::datastore::ArrayStoreConfig BTreeBucketDatabase::make_default_array_store_config() {
+    return ReplicaStore::optimizedConfigForHugePage(1023, vespalib::alloc::MemoryAllocator::HUGEPAGE_SIZE,
+                                                    4 * 1024, 8 * 1024, 0.2).enable_free_lists(true);
+}
+
 namespace {
 
 Entry entry_from_replica_array_ref(const BucketId& id, uint32_t gc_timestamp, ConstArrayRef<BucketCopy> replicas) {
@@ -232,11 +236,11 @@ void BTreeBucketDatabase::remove(const BucketId& bucket) {
  * skip scanning through many disjoint subtrees.
  */
 BTreeBucketDatabase::BTree::ConstIterator
-BTreeBucketDatabase::find_parents_internal(const document::BucketId& bucket,
+BTreeBucketDatabase::find_parents_internal(const BTree::FrozenView& frozen_view,
+                                           const document::BucketId& bucket,
                                            std::vector<Entry>& entries) const
 {
     const uint64_t bucket_key = bucket.toKey();
-    const auto frozen_view = _tree.getFrozenView();
     auto iter = frozen_view.begin();
     // Start at the root level, descending towards the bucket itself.
     // Try skipping as many levels of the tree as possible as we go.
@@ -255,6 +259,16 @@ BTreeBucketDatabase::find_parents_internal(const document::BucketId& bucket,
     return iter;
 }
 
+void BTreeBucketDatabase::find_parents_and_self_internal(const BTree::FrozenView& frozen_view,
+                                                         const document::BucketId& bucket,
+                                                         std::vector<Entry>& entries) const
+{
+    auto iter = find_parents_internal(frozen_view, bucket, entries);
+    if (iter.valid() && iter.getKey() == bucket.toKey()) {
+        entries.emplace_back(entry_from_iterator(iter));
+    }
+}
+
 /*
  * Note: due to legacy API reasons, iff the requested bucket itself exists in the
  * tree, it will be returned in the result set. I.e. it returns all the nodes on
@@ -263,16 +277,15 @@ BTreeBucketDatabase::find_parents_internal(const document::BucketId& bucket,
 void BTreeBucketDatabase::getParents(const BucketId& bucket,
                                      std::vector<Entry>& entries) const
 {
-    auto iter = find_parents_internal(bucket, entries);
-    if (iter.valid() && iter.getKey() == bucket.toKey()) {
-        entries.emplace_back(entry_from_iterator(iter));
-    }
+    auto view = _tree.getFrozenView();
+    find_parents_and_self_internal(view, bucket, entries);
 }
 
 void BTreeBucketDatabase::getAll(const BucketId& bucket,
                                  std::vector<Entry>& entries) const
 {
-    auto iter = find_parents_internal(bucket, entries);
+    auto view = _tree.getFrozenView();
+    auto iter = find_parents_internal(view, bucket, entries);
     // `iter` is already pointing at, or beyond, one of the bucket's subtrees.
     for (; iter.valid(); ++iter) {
         auto candidate = BucketId(BucketId::keyToBucketId(iter.getKey()));
@@ -510,4 +523,18 @@ void BTreeBucketDatabase::print(std::ostream& out, bool verbose,
     (void)indent;
 }
 
+BTreeBucketDatabase::ReadGuardImpl::ReadGuardImpl(const BTreeBucketDatabase& db)
+    : _db(&db),
+      _guard(_db->_generation_handler.takeGuard()),
+      _frozen_view(_db->_tree.getFrozenView())
+{}
+
+BTreeBucketDatabase::ReadGuardImpl::~ReadGuardImpl() = default;
+
+void BTreeBucketDatabase::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket,
+                                                               std::vector<Entry>& entries) const
+{
+    _db->find_parents_and_self_internal(_frozen_view, bucket, entries);
+}
+
 }
diff --git a/storage/src/vespa/storage/bucketdb/btree_bucket_database.h b/storage/src/vespa/storage/bucketdb/btree_bucket_database.h
index eb527071ad7..5c69b57956b 100644
--- a/storage/src/vespa/storage/bucketdb/btree_bucket_database.h
+++ b/storage/src/vespa/storage/bucketdb/btree_bucket_database.h
@@ -17,13 +17,9 @@ namespace storage {
  * this is an uint32 we cheekily mangle it into the value, i.e. each bucket key maps to a
  * composite key of (gc_timestamp_u32 << 32) | array_ref_u32.
  *
- * This is _not_ yet production ready, for several reasons:
- *   - Underlying ArrayStore does not have freelists enabled for replica entry reuse
- *   - Current API design for mutable forEach requires O(n) tree structure mutations instead
- *     of changing the tree in bulk and reusing ArrayStore refs et al. Needs a redesign.
- *
- * Also note that the DB is currently _not_ thread safe, as read snapshotting is not yet defined
- * or exposed.
+ * Readers from contexts that are not guaranteed to be the main distributor thread MUST
+ * only access the database via an acquired read guard.
+ * Writing MUST only take place from the main distributor thread.
  */
 // TODO create and use a new DB interface with better bulk loading, snapshot and iteration support
 class BTreeBucketDatabase : public BucketDatabase {
@@ -66,11 +62,34 @@ private:
     ConstEntryRef const_entry_ref_from_iterator(const BTree::ConstIterator& iter) const;
     document::BucketId bucket_from_valid_iterator(const BTree::ConstIterator& iter) const;
     void commit_tree_changes();
-    BTree::ConstIterator find_parents_internal(const document::BucketId& bucket,
+    BTree::ConstIterator find_parents_internal(const BTree::FrozenView& frozen_view,
+                                               const document::BucketId& bucket,
                                                std::vector<Entry>& entries) const;
+    void find_parents_and_self_internal(const BTree::FrozenView& frozen_view,
+                                        const document::BucketId& bucket,
+                                        std::vector<Entry>& entries) const;
+
+    static search::datastore::ArrayStoreConfig make_default_array_store_config();
+
+    class ReadGuardImpl : public ReadGuard {
+        const BTreeBucketDatabase* _db;
+        GenerationHandler::Guard   _guard;
+        BTree::FrozenView          _frozen_view;
+    public:
+        explicit ReadGuardImpl(const BTreeBucketDatabase& db);
+        ~ReadGuardImpl() override;
+
+        void find_parents_and_self(const document::BucketId& bucket,
+                                   std::vector<Entry>& entries) const override;
+    };
+
+    friend class ReadGuardImpl;
+
+    std::unique_ptr<ReadGuard> acquire_read_guard() const override {
+        return std::make_unique<ReadGuardImpl>(*this);
+    }
 
     friend struct BTreeBuilderMerger;
-    friend struct BTreeMergingBuilder;
     friend struct BTreeTrailingInserter;
 };
 
diff --git a/storage/src/vespa/storage/bucketdb/bucketdatabase.h b/storage/src/vespa/storage/bucketdb/bucketdatabase.h
index d12c2d82972..0659d02cc15 100644
--- a/storage/src/vespa/storage/bucketdb/bucketdatabase.h
+++ b/storage/src/vespa/storage/bucketdb/bucketdatabase.h
@@ -232,6 +232,23 @@ public:
             const document::BucketId& bid);
 
     virtual uint32_t childCount(const document::BucketId&) const = 0;
+
+    struct ReadGuard {
+        ReadGuard() = default;
+        virtual ~ReadGuard() = default;
+        
+        ReadGuard(ReadGuard&&) = delete;
+        ReadGuard& operator=(ReadGuard&&) = delete;
+        ReadGuard(const ReadGuard&) = delete;
+        ReadGuard& operator=(const ReadGuard&) = delete;
+
+        virtual void find_parents_and_self(const document::BucketId& bucket,
+                                           std::vector<Entry>& entries) const = 0;
+    };
+
+    virtual std::unique_ptr<ReadGuard> acquire_read_guard() const {
+        return std::unique_ptr<ReadGuard>();
+    }
 };
 
 template <typename BucketInfoType>
diff --git a/storage/src/vespa/storage/bucketdb/mapbucketdatabase.cpp b/storage/src/vespa/storage/bucketdb/mapbucketdatabase.cpp
index 9dad421324e..463e4a4b8ce 100644
--- a/storage/src/vespa/storage/bucketdb/mapbucketdatabase.cpp
+++ b/storage/src/vespa/storage/bucketdb/mapbucketdatabase.cpp
@@ -584,4 +584,14 @@ MapBucketDatabase::print(std::ostream& out, bool verbose,
     out << ')';
 }
 
+std::unique_ptr<BucketDatabase::ReadGuard> MapBucketDatabase::acquire_read_guard() const {
+    return std::make_unique<ReadGuardImpl>(*this);
+}
+
+void MapBucketDatabase::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket,
+                                                             std::vector<Entry>& entries) const
+{
+    _db->getParents(bucket, entries);
+}
+
 } // storage
diff --git a/storage/src/vespa/storage/bucketdb/mapbucketdatabase.h b/storage/src/vespa/storage/bucketdb/mapbucketdatabase.h
index df3e055bd7a..b295be588a6 100644
--- a/storage/src/vespa/storage/bucketdb/mapbucketdatabase.h
+++ b/storage/src/vespa/storage/bucketdb/mapbucketdatabase.h
@@ -28,6 +28,8 @@ public:
 
     document::BucketId getAppropriateBucket(uint16_t minBits, const document::BucketId& bid) override;
     void print(std::ostream& out, bool verbose, const std::string& indent) const override;
+
+    std::unique_ptr<ReadGuard> acquire_read_guard() const override;
 private:
     struct E {
         E() : value(-1), e_0(-1), e_1(-1) {};
@@ -62,6 +64,17 @@ private:
     uint8_t getHighestSplitBit(int index, uint8_t bitCount, const document::BucketId& bid, uint8_t minCount);
     uint32_t childCountImpl(int index, uint8_t bitCount, const document::BucketId& b) const;
 
+    // NOT thread-safe for concurrent reads!
+    class ReadGuardImpl : public ReadGuard {
+        const MapBucketDatabase* _db;
+    public:
+        explicit ReadGuardImpl(const MapBucketDatabase& db) : _db(&db) {}
+        ~ReadGuardImpl() override = default;
+
+        void find_parents_and_self(const document::BucketId& bucket,
+                                   std::vector<Entry>& entries) const override;
+    };
+
     uint32_t allocate();
     uint32_t allocateValue(const document::BucketId& bid);