Merge pull request #13819 from vespa-engine/vekterli/basic-snapshot-support-for-content-node-bucket-db

Vekterli/basic snapshot support for content node bucket db

18 files changed, 314 insertions, 94 deletions

diff --git a/storage/src/tests/bucketdb/lockablemaptest.cpp b/storage/src/tests/bucketdb/lockablemaptest.cpp
index c5d22b6e6b5..709e158e531 100644
--- a/storage/src/tests/bucketdb/lockablemaptest.cpp
+++ b/storage/src/tests/bucketdb/lockablemaptest.cpp
@@ -6,6 +6,7 @@
 #include <vespa/storage/bucketdb/lockablemap.hpp>
 #include <vespa/storage/bucketdb/btree_lockable_map.hpp>
 #include <vespa/vespalib/gtest/gtest.h>
+#include <gmock/gmock.h>
 
 #include <vespa/log/log.h>
 LOG_SETUP(".lockable_map_test");
@@ -200,13 +201,23 @@ EntryProcessor<Map>::~EntryProcessor() = default;
 
 template <typename Map>
 struct ConstProcessor {
+    mutable uint32_t count;
     mutable std::vector<std::string> log;
+    mutable std::vector<typename Map::Decision> behaviour;
+
+    ConstProcessor();
+    explicit ConstProcessor(std::vector<typename Map::Decision> decisions);
+    ~ConstProcessor();
 
     typename Map::Decision operator()(uint64_t key, const A& a) const {
         std::ostringstream ost;
         ost << key << " - " << a;
         log.push_back(ost.str());
-        return Map::CONTINUE;
+        auto d = Map::Decision::CONTINUE;
+        if (behaviour.size() > count) {
+            d = behaviour[count++];
+        }
+        return d;
     }
 
     std::string toString() {
@@ -218,6 +229,17 @@ struct ConstProcessor {
     }
 };
 
+template <typename Map>
+ConstProcessor<Map>::ConstProcessor()
+    : count(0), log(), behaviour() {}
+
+template <typename Map>
+ConstProcessor<Map>::ConstProcessor(std::vector<typename Map::Decision> decisions)
+    : count(0), log(), behaviour(std::move(decisions)) {}
+
+template <typename Map>
+ConstProcessor<Map>::~ConstProcessor() = default;
+
 }
 
 TYPED_TEST(LockableMapTest, iterating) {
@@ -300,17 +322,21 @@ TYPED_TEST(LockableMapTest, chunked_iteration_is_transparent_across_chunk_sizes)
     map.insert(16, A(1, 2, 3), "foo", preExisted);
     map.insert(11, A(4, 6, 0), "foo", preExisted);
     map.insert(14, A(42, 0, 0), "foo", preExisted);
-    NonConstProcessor<TypeParam> ncproc; // Increments 2nd value in all entries.
-    // for_each_chunked with chunk size of 1
-    map.for_each_chunked(std::ref(ncproc), "foo", 1us, 1);
-    EXPECT_EQ(A(4, 7, 0), *map.get(11, "foo"));
-    EXPECT_EQ(A(42, 1, 0), *map.get(14, "foo"));
-    EXPECT_EQ(A(1, 3, 3), *map.get(16, "foo"));
-    // for_each_chunked with chunk size larger than db size
-    map.for_each_chunked(std::ref(ncproc), "foo", 1us, 100);
-    EXPECT_EQ(A(4, 8, 0), *map.get(11, "foo"));
-    EXPECT_EQ(A(42, 2, 0), *map.get(14, "foo"));
-    EXPECT_EQ(A(1, 4, 3), *map.get(16, "foo"));
+    std::string expected("11 - A(4, 6, 0)\n"
+                         "14 - A(42, 0, 0)\n"
+                         "16 - A(1, 2, 3)\n");
+    {
+        ConstProcessor<TypeParam> cproc;
+        // for_each_chunked with chunk size of 1
+        map.for_each_chunked(std::ref(cproc), "foo", 1us, 1);
+        EXPECT_EQ(expected, cproc.toString());
+    }
+    {
+        ConstProcessor<TypeParam> cproc;
+        // for_each_chunked with chunk size larger than db size
+        map.for_each_chunked(std::ref(cproc), "foo", 1us, 100);
+        EXPECT_EQ(expected, cproc.toString());
+    }
 }
 
 TYPED_TEST(LockableMapTest, can_abort_during_chunked_iteration) {
@@ -323,13 +349,29 @@ TYPED_TEST(LockableMapTest, can_abort_during_chunked_iteration) {
     std::vector<typename TypeParam::Decision> decisions;
     decisions.push_back(TypeParam::CONTINUE);
     decisions.push_back(TypeParam::ABORT);
-    EntryProcessor<TypeParam> proc(decisions);
+    ConstProcessor<TypeParam> proc(std::move(decisions));
     map.for_each_chunked(std::ref(proc), "foo", 1us, 100);
     std::string expected("11 - A(4, 6, 0)\n"
-            "14 - A(42, 0, 0)\n");
+                         "14 - A(42, 0, 0)\n");
     EXPECT_EQ(expected, proc.toString());
 }
 
+TYPED_TEST(LockableMapTest, can_iterate_via_read_guard) {
+    TypeParam map;
+    bool pre_existed;
+    map.insert(16, A(1, 2, 3), "foo", pre_existed);
+    map.insert(11, A(4, 6, 0), "foo", pre_existed);
+    map.insert(14, A(42, 0, 0), "foo", pre_existed);
+    std::string expected("11 - A(4, 6, 0)\n"
+                         "14 - A(42, 0, 0)\n"
+                         "16 - A(1, 2, 3)\n");
+
+    ConstProcessor<TypeParam> cproc;
+    auto guard = map.acquire_read_guard();
+    guard->for_each(std::ref(cproc));
+    EXPECT_EQ(expected, cproc.toString());
+}
+
 TYPED_TEST(LockableMapTest, find_buckets_simple) {
     TypeParam map;
 
@@ -509,6 +551,18 @@ TYPED_TEST(LockableMapTest, find_all) {
     EXPECT_EQ(1, results.size());
 
     EXPECT_EQ(A(9,10,11), *results[id9.stripUnused()]); // sub bucket
+
+    // Make sure we clear any existing bucket locks before we continue, or test will deadlock
+    // if running with legacy (non-snapshot capable) DB implementation.
+    results.clear();
+    // Results should be equal when using read guard
+    auto guard = map.acquire_read_guard();
+
+    auto guard_results = guard->find_parents_self_and_children(BucketId(17, 0x1aaaa));
+    EXPECT_THAT(guard_results, ElementsAre(A(1,2,3), A(5,6,7), A(6,7,8), A(7,8,9)));
+
+    guard_results = guard->find_parents_self_and_children(BucketId(16, 0xffff));
+    EXPECT_THAT(guard_results, ElementsAre(A(9,10,11)));
 }
 
 TYPED_TEST(LockableMapTest, find_all_2) { // Ticket 3121525
diff --git a/storage/src/tests/distributor/btree_bucket_database_test.cpp b/storage/src/tests/distributor/btree_bucket_database_test.cpp
index a2518272a7f..5283cc39da8 100644
--- a/storage/src/tests/distributor/btree_bucket_database_test.cpp
+++ b/storage/src/tests/distributor/btree_bucket_database_test.cpp
@@ -37,8 +37,10 @@ struct BTreeReadGuardTest : Test {
 TEST_F(BTreeReadGuardTest, guard_does_not_observe_new_entries) {
     auto guard = _db.acquire_read_guard();
     _db.update(BucketDatabase::Entry(BucketId(16, 16), BI(1, 1234)));
-    std::vector<BucketDatabase::Entry> entries;
-    guard->find_parents_and_self(BucketId(16, 16), entries);
+
+    auto entries = guard->find_parents_and_self(BucketId(16, 16));
+    EXPECT_EQ(entries.size(), 0U);
+    entries = guard->find_parents_self_and_children(BucketId(16, 16));
     EXPECT_EQ(entries.size(), 0U);
 }
 
@@ -47,8 +49,12 @@ TEST_F(BTreeReadGuardTest, guard_observes_entries_alive_at_acquire_time) {
     _db.update(BucketDatabase::Entry(bucket, BI(1, 1234)));
     auto guard = _db.acquire_read_guard();
     _db.remove(bucket);
-    std::vector<BucketDatabase::Entry> entries;
-    guard->find_parents_and_self(bucket, entries);
+
+    auto entries = guard->find_parents_and_self(bucket);
+    ASSERT_EQ(entries.size(), 1U);
+    EXPECT_EQ(entries[0].getBucketInfo(), BI(1, 1234));
+
+    entries = guard->find_parents_self_and_children(bucket);
     ASSERT_EQ(entries.size(), 1U);
     EXPECT_EQ(entries[0].getBucketInfo(), BI(1, 1234));
 }
diff --git a/storage/src/tests/distributor/bucketdatabasetest.cpp b/storage/src/tests/distributor/bucketdatabasetest.cpp
index 0b832699364..7f9825d32fc 100644
--- a/storage/src/tests/distributor/bucketdatabasetest.cpp
+++ b/storage/src/tests/distributor/bucketdatabasetest.cpp
@@ -173,8 +173,7 @@ BucketDatabaseTest::doFindParents(const std::vector<document::BucketId>& ids,
     // 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);
+    auto checked_entries = db().acquire_read_guard()->find_parents_and_self(searchId);
     if (entries != checked_entries) {
         return "Mismatch between results from getParents() and ReadGuard!";
     }
diff --git a/storage/src/tests/distributor/bucketdbupdatertest.cpp b/storage/src/tests/distributor/bucketdbupdatertest.cpp
index 495b6fb570e..4b8ea56e5ca 100644
--- a/storage/src/tests/distributor/bucketdbupdatertest.cpp
+++ b/storage/src/tests/distributor/bucketdbupdatertest.cpp
@@ -2769,8 +2769,7 @@ struct BucketDBUpdaterSnapshotTest : BucketDBUpdaterTest {
         uint32_t found_buckets = 0;
         for_each_bucket(repo, [&](const auto& space, const auto& entry) {
             if (space == bucket_space) {
-                std::vector<BucketDatabase::Entry> entries;
-                guard->find_parents_and_self(entry.getBucketId(), entries);
+                auto entries = guard->find_parents_and_self(entry.getBucketId());
                 if (entries.size() == 1) {
                     ++found_buckets;
                 }
diff --git a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h
index a6855532e30..ae4c48ed22f 100644
--- a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h
+++ b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h
@@ -1,6 +1,7 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #pragma once
 
+#include "read_guard.h"
 #include <vespa/document/bucket/bucketid.h>
 #include <vespa/vespalib/stllike/hash_map.h>
 #include <vespa/vespalib/stllike/hash_set.h>
@@ -161,7 +162,7 @@ public:
      *
      * Type erasure of functor needed due to virtual indirection.
      */
-    void for_each_chunked(std::function<Decision(uint64_t, ValueT&)> func,
+    void for_each_chunked(std::function<Decision(uint64_t, const ValueT&)> func,
                           const char* clientId,
                           vespalib::duration yieldTime = 10us,
                           uint32_t chunkSize = DEFAULT_CHUNK_SIZE)
@@ -185,6 +186,10 @@ public:
         do_for_each(std::move(func), clientId, first, last);
     }
 
+    std::unique_ptr<bucketdb::ReadGuard<ValueT>> acquire_read_guard() const {
+        return do_acquire_read_guard();
+    }
+
     [[nodiscard]] virtual size_type size() const noexcept = 0;
     [[nodiscard]] virtual size_type getMemoryUsage() const noexcept = 0;
     [[nodiscard]] virtual bool empty() const noexcept = 0;
@@ -194,7 +199,7 @@ public:
     virtual void print(std::ostream& out, bool verbose, const std::string& indent) const = 0;
 private:
     virtual void unlock(const key_type& key) = 0; // Only for bucket lock guards
-    virtual void do_for_each_chunked(std::function<Decision(uint64_t, ValueT&)> func,
+    virtual void do_for_each_chunked(std::function<Decision(uint64_t, const ValueT&)> func,
                                      const char* clientId,
                                      vespalib::duration yieldTime,
                                      uint32_t chunkSize) = 0;
@@ -206,6 +211,7 @@ private:
                              const char* clientId,
                              const key_type& first,
                              const key_type& last) = 0;
+    virtual std::unique_ptr<bucketdb::ReadGuard<ValueT>> do_acquire_read_guard() const = 0;
 };
 
 template <typename ValueT>
diff --git a/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp b/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp
index 9634d6d0953..7bf78c8ba7e 100644
--- a/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp
+++ b/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp
@@ -198,8 +198,9 @@ public:
     explicit ReadGuardImpl(const BTreeBucketDatabase& db);
     ~ReadGuardImpl() override;
 
-    void find_parents_and_self(const document::BucketId& bucket,
-                               std::vector<Entry>& entries) const override;
+    std::vector<Entry> find_parents_and_self(const document::BucketId& bucket) const override;
+    std::vector<Entry> find_parents_self_and_children(const document::BucketId& bucket) const override;
+    void for_each(std::function<void(uint64_t, const Entry&)> func) const override;
     [[nodiscard]] uint64_t generation() const noexcept override;
 };
 
@@ -209,12 +210,26 @@ BTreeBucketDatabase::ReadGuardImpl::ReadGuardImpl(const BTreeBucketDatabase& db)
 
 BTreeBucketDatabase::ReadGuardImpl::~ReadGuardImpl() = default;
 
-void BTreeBucketDatabase::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket,
-                                                               std::vector<Entry>& entries) const
-{
+std::vector<Entry>
+BTreeBucketDatabase::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket) const {
+    std::vector<Entry> entries;
     _snapshot.find_parents_and_self<ByValue>(bucket, [&entries]([[maybe_unused]] uint64_t key, Entry entry){
         entries.emplace_back(std::move(entry));
     });
+    return entries;
+}
+
+std::vector<Entry>
+BTreeBucketDatabase::ReadGuardImpl::find_parents_self_and_children(const document::BucketId& bucket) const {
+    std::vector<Entry> entries;
+    _snapshot.find_parents_self_and_children<ByValue>(bucket, [&entries]([[maybe_unused]] uint64_t key, Entry entry){
+        entries.emplace_back(std::move(entry));
+    });
+    return entries;
+}
+
+void BTreeBucketDatabase::ReadGuardImpl::for_each(std::function<void(uint64_t, const Entry&)> func) const {
+    _snapshot.for_each<ByValue>(std::move(func));
 }
 
 uint64_t BTreeBucketDatabase::ReadGuardImpl::generation() const noexcept {
diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h
index 3d58b85b063..ba2e6ca28a0 100644
--- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h
+++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h
@@ -101,11 +101,14 @@ private:
         WaiterMap _map;
     };
 
-    mutable std::mutex      _lock;
-    std::condition_variable _cond;
-    std::unique_ptr<GenericBTreeBucketDatabase<ValueTraits>> _impl;
-    LockIdSet         _lockedKeys;
-    LockWaiters       _lockWaiters;
+    class ReadGuardImpl;
+    using ImplType = GenericBTreeBucketDatabase<ValueTraits>;
+
+    mutable std::mutex        _lock;
+    std::condition_variable   _cond;
+    std::unique_ptr<ImplType> _impl;
+    LockIdSet                 _lockedKeys;
+    LockWaiters               _lockWaiters;
 
     void unlock(const key_type& key) override;
     bool findNextKey(key_type& key, mapped_type& val, const char* clientId,
@@ -123,11 +126,13 @@ private:
                      const key_type& first,
                      const key_type& last) override;
 
-    void do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func,
+    void do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func,
                              const char* client_id,
                              vespalib::duration yield_time,
                              uint32_t chunk_size) override;
 
+    std::unique_ptr<ReadGuard<T>> do_acquire_read_guard() const override;
+
     /**
      * Process up to `chunk_size` bucket database entries from--and possibly
      * including--the bucket pointed to by `key`.
@@ -139,7 +144,7 @@ private:
      * Modifies `key` in-place to point to the next key to process for the next
      * invocation of this function.
      */
-    bool processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func,
+    bool processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func,
                           key_type& key,
                           const char* client_id,
                           uint32_t chunk_size);
diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp
index 18eae405dc6..e8c91f04a9e 100644
--- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp
+++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp
@@ -307,7 +307,7 @@ void BTreeLockableMap<T>::do_for_each(std::function<Decision(uint64_t, const map
 }
 
 template <typename T>
-bool BTreeLockableMap<T>::processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func,
+bool BTreeLockableMap<T>::processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func,
                                            key_type& key,
                                            const char* client_id,
                                            const uint32_t chunk_size)
@@ -328,7 +328,7 @@ bool BTreeLockableMap<T>::processNextChunk(std::function<Decision(uint64_t, mapp
 }
 
 template <typename T>
-void BTreeLockableMap<T>::do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func,
+void BTreeLockableMap<T>::do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func,
                                               const char* client_id,
                                               vespalib::duration yield_time,
                                               uint32_t chunk_size)
@@ -347,6 +347,66 @@ void BTreeLockableMap<T>::do_for_each_chunked(std::function<Decision(uint64_t, m
 }
 
 template <typename T>
+class BTreeLockableMap<T>::ReadGuardImpl final : public bucketdb::ReadGuard<T> {
+    typename ImplType::ReadSnapshot _snapshot;
+public:
+    explicit ReadGuardImpl(const BTreeLockableMap<T>& db);
+    ~ReadGuardImpl() override;
+
+    std::vector<T> find_parents_and_self(const document::BucketId& bucket) const override;
+    std::vector<T> find_parents_self_and_children(const document::BucketId& bucket) const override;
+    void for_each(std::function<void(uint64_t, const T&)> func) const override;
+    [[nodiscard]] uint64_t generation() const noexcept override;
+};
+
+template <typename T>
+BTreeLockableMap<T>::ReadGuardImpl::ReadGuardImpl(const BTreeLockableMap<T>& db)
+    : _snapshot(*db._impl)
+{}
+
+template <typename T>
+BTreeLockableMap<T>::ReadGuardImpl::~ReadGuardImpl() = default;
+
+template <typename T>
+std::vector<T>
+BTreeLockableMap<T>::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket) const {
+    std::vector<T> entries;
+    _snapshot.template find_parents_and_self<ByConstRef>(
+            bucket,
+            [&entries]([[maybe_unused]] uint64_t key, const T& entry){
+                entries.emplace_back(entry);
+            });
+    return entries;
+}
+
+template <typename T>
+std::vector<T>
+BTreeLockableMap<T>::ReadGuardImpl::find_parents_self_and_children(const document::BucketId& bucket) const {
+    std::vector<T> entries;
+    _snapshot.template find_parents_self_and_children<ByConstRef>(
+            bucket,
+            [&entries]([[maybe_unused]] uint64_t key, const T& entry){
+                entries.emplace_back(entry);
+            });
+    return entries;
+}
+
+template <typename T>
+void BTreeLockableMap<T>::ReadGuardImpl::for_each(std::function<void(uint64_t, const T&)> func) const {
+    _snapshot.template for_each<ByConstRef>(std::move(func));
+}
+
+template <typename T>
+uint64_t BTreeLockableMap<T>::ReadGuardImpl::generation() const noexcept {
+    return _snapshot.generation();
+}
+
+template <typename T>
+std::unique_ptr<ReadGuard<T>> BTreeLockableMap<T>::do_acquire_read_guard() const {
+    return std::make_unique<ReadGuardImpl>(*this);
+}
+
+template <typename T>
 void BTreeLockableMap<T>::print(std::ostream& out, bool verbose,
                                 const std::string& indent) const
 {
diff --git a/storage/src/vespa/storage/bucketdb/bucketmanager.cpp b/storage/src/vespa/storage/bucketdb/bucketmanager.cpp
index fcb019fb6bc..530c1236436 100644
--- a/storage/src/vespa/storage/bucketdb/bucketmanager.cpp
+++ b/storage/src/vespa/storage/bucketdb/bucketmanager.cpp
@@ -174,12 +174,11 @@ namespace {
         std::vector<Count> disk;
         uint32_t lowestUsedBit;
 
-        MetricsUpdater(uint16_t diskCnt)
+        explicit MetricsUpdater(uint16_t diskCnt)
             : diskCount(diskCnt), disk(diskCnt), lowestUsedBit(58) {}
 
-        StorBucketDatabase::Decision operator()(
-                document::BucketId::Type bucketId,
-                const StorBucketDatabase::Entry& data)
+        void operator()(document::BucketId::Type bucketId,
+                        const StorBucketDatabase::Entry& data)
         {
             document::BucketId bucket(
                     document::BucketId::keyToBucketId(bucketId));
@@ -200,8 +199,6 @@ namespace {
                     lowestUsedBit = bucket.getUsedBits();
                 }
             }
-
-            return StorBucketDatabase::Decision::CONTINUE;
         };
 
         void add(const MetricsUpdater& rhs) {
@@ -241,7 +238,8 @@ BucketManager::updateMetrics(bool updateDocCount)
         MetricsUpdater total(diskCount);
         for (auto& space : _component.getBucketSpaceRepo()) {
             MetricsUpdater m(diskCount);
-            space.second->bucketDatabase().for_each_chunked(std::ref(m), "BucketManager::updateMetrics");
+            auto guard = space.second->bucketDatabase().acquire_read_guard();
+            guard->for_each(std::ref(m));
             total.add(m);
             if (updateDocCount) {
                 auto bm = _metrics->bucket_spaces.find(space.first);
@@ -270,8 +268,7 @@ BucketManager::updateMetrics(bool updateDocCount)
 void BucketManager::updateMinUsedBits()
 {
     MetricsUpdater m(_component.getDiskCount());
-    _component.getBucketSpaceRepo().forEachBucketChunked(
-            m, "BucketManager::updateMetrics");
+    _component.getBucketSpaceRepo().for_each_bucket(std::ref(m));
     // When going through to get sizes, we also record min bits
     MinimumUsedBitsTracker& bitTracker(_component.getMinUsedBitsTracker());
     if (bitTracker.getMinUsedBits() != m.lowestUsedBit) {
@@ -339,9 +336,7 @@ namespace {
     public:
         explicit BucketDBDumper(vespalib::XmlOutputStream& xos) : _xos(xos) {}
 
-        StorBucketDatabase::Decision operator()(
-                uint64_t bucketId, const StorBucketDatabase::Entry& info)
-        {
+        void operator()(uint64_t bucketId, const StorBucketDatabase::Entry& info) {
             using namespace vespalib::xml;
             document::BucketId bucket(
                     document::BucketId::keyToBucketId(bucketId));
@@ -355,7 +350,6 @@ namespace {
             info.getBucketInfo().printXml(_xos);
             _xos << XmlAttribute("disk", info.disk);
             _xos << XmlEndTag();
-            return StorBucketDatabase::Decision::CONTINUE;
         };
     };
 }
@@ -377,8 +371,8 @@ BucketManager::reportStatus(std::ostream& out,
             xmlReporter << XmlTag("bucket-space")
                         << XmlAttribute("name", document::FixedBucketSpaces::to_string(space.first));
             BucketDBDumper dumper(xmlReporter.getStream());
-            _component.getBucketSpaceRepo().get(space.first).bucketDatabase().for_each_chunked(
-                    std::ref(dumper), "BucketManager::reportStatus");
+            auto guard = _component.getBucketSpaceRepo().get(space.first).bucketDatabase().acquire_read_guard();
+            guard->for_each(std::ref(dumper));
             xmlReporter << XmlEndTag();
         }
         xmlReporter << XmlEndTag();
@@ -398,7 +392,7 @@ BucketManager::dump(std::ostream& out) const
 {
     vespalib::XmlOutputStream xos(out);
     BucketDBDumper dumper(xos);
-    _component.getBucketSpaceRepo().forEachBucketChunked(dumper, "BucketManager::dump");
+    _component.getBucketSpaceRepo().for_each_bucket(std::ref(dumper));
 }
 
 
diff --git a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h
index 8bc7a3379b3..977aeb8f925 100644
--- a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h
+++ b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h
@@ -71,13 +71,6 @@ public:
     GenericBTreeBucketDatabase(GenericBTreeBucketDatabase&&) = delete;
     GenericBTreeBucketDatabase& operator=(GenericBTreeBucketDatabase&&) = delete;
 
-    // TODO move
-    struct EntryProcessor {
-        virtual ~EntryProcessor() = default;
-        /** Return false to stop iterating. */
-        virtual bool process(const typename DataStoreTraitsT::ConstValueRef& e) = 0;
-    };
-
     ValueType entry_from_iterator(const BTreeConstIterator& iter) const;
     ConstValueRef const_value_ref_from_valid_iterator(const BTreeConstIterator& iter) const;
 
@@ -103,14 +96,10 @@ public:
     bool update_by_raw_key(uint64_t bucket_key, const ValueType& new_entry);
 
     template <typename IterValueExtractor, typename Func>
-    void find_parents_and_self(const document::BucketId& bucket,
-                               Func func) const;
+    void find_parents_and_self(const document::BucketId& bucket, Func func) const;
 
     template <typename IterValueExtractor, typename Func>
-    void find_parents_self_and_children(const document::BucketId& bucket,
-                                        Func func) const;
-
-    void for_each(EntryProcessor& proc, const document::BucketId& after) const;
+    void find_parents_self_and_children(const document::BucketId& bucket, Func func) const;
 
     document::BucketId getAppropriateBucket(uint16_t minBits, const document::BucketId& bid) const;
 
@@ -136,6 +125,10 @@ public:
 
         template <typename IterValueExtractor, typename Func>
         void find_parents_and_self(const document::BucketId& bucket, Func func) const;
+        template <typename IterValueExtractor, typename Func>
+        void find_parents_self_and_children(const document::BucketId& bucket, Func func) const;
+        template <typename IterValueExtractor, typename Func>
+        void for_each(Func func) const;
         [[nodiscard]] uint64_t generation() const noexcept;
     };
 private:
@@ -148,6 +141,11 @@ private:
     void find_parents_and_self_internal(const typename BTree::FrozenView& frozen_view,
                                         const document::BucketId& bucket,
                                         Func func) const;
+    template <typename IterValueExtractor, typename Func>
+    void find_parents_self_and_children_internal(const typename BTree::FrozenView& frozen_view,
+                                                 const document::BucketId& bucket,
+                                                 Func func) const;
+
     void commit_tree_changes();
 
     template <typename DataStoreTraitsT2> friend struct BTreeBuilderMerger;
diff --git a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp
index adaf402e4d1..5fb673a1440 100644
--- a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp
+++ b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp
@@ -250,12 +250,12 @@ void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_and_self(
 
 template <typename DataStoreTraitsT>
 template <typename IterValueExtractor, typename Func>
-void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_children(
+void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_children_internal(
+        const typename BTree::FrozenView& frozen_view,
         const BucketId& bucket,
         Func func) const
 {
-    auto view = _tree.getFrozenView();
-    auto iter = find_parents_internal<IterValueExtractor>(view, bucket, func);
+    auto iter = find_parents_internal<IterValueExtractor>(frozen_view, bucket, func);
     // `iter` is already pointing at, or beyond, one of the bucket's subtrees.
     for (; iter.valid(); ++iter) {
         auto candidate = BucketId(BucketId::keyToBucketId(iter.getKey()));
@@ -268,6 +268,16 @@ void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_childre
 }
 
 template <typename DataStoreTraitsT>
+template <typename IterValueExtractor, typename Func>
+void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_children(
+        const BucketId& bucket,
+        Func func) const
+{
+    auto view = _tree.getFrozenView();
+    find_parents_self_and_children_internal<IterValueExtractor>(view, bucket, std::move(func));
+}
+
+template <typename DataStoreTraitsT>
 typename GenericBTreeBucketDatabase<DataStoreTraitsT>::ValueType
 GenericBTreeBucketDatabase<DataStoreTraitsT>::get(const BucketId& bucket) const {
     return entry_from_iterator(_tree.find(bucket.toKey()));
@@ -323,19 +333,6 @@ bool GenericBTreeBucketDatabase<DataStoreTraitsT>::update(const BucketId& bucket
     return update_by_raw_key(bucket.toKey(), new_entry);
 }
 
-// TODO need snapshot read with guarding
-// FIXME semantics of for-each in judy and bit tree DBs differ, former expects lbound, latter ubound..!
-// FIXME but bit-tree code says "lowerBound" in impl and "after" in declaration???
-template <typename DataStoreTraitsT>
-void GenericBTreeBucketDatabase<DataStoreTraitsT>::for_each(EntryProcessor& proc, const BucketId& after) const {
-    for (auto iter = _tree.upperBound(after.toKey()); iter.valid(); ++iter) {
-        // TODO memory fencing once we use snapshots!
-        if (!proc.process(DataStoreTraitsT::unwrap_const_ref_from_key_value(_store, iter.getKey(), iter.getData()))) {
-            break;
-        }
-    }
-}
-
 /*
  * Returns the bucket ID which, based on the buckets already existing in the DB,
  * is the most specific location in the tree in which it should reside. This may
@@ -541,6 +538,24 @@ void GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::find_parents_an
 }
 
 template <typename DataStoreTraitsT>
+template <typename IterValueExtractor, typename Func>
+void GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::find_parents_self_and_children(
+        const BucketId& bucket,
+        Func func) const
+{
+    _db->find_parents_self_and_children_internal<IterValueExtractor>(_frozen_view, bucket, std::move(func));
+}
+
+template <typename DataStoreTraitsT>
+template <typename IterValueExtractor, typename Func>
+void GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::for_each(Func func) const {
+    for (auto iter = _frozen_view.begin(); iter.valid(); ++iter) {
+        // Iterator value extractor implicitly inserts any required memory fences for value.
+        func(iter.getKey(), IterValueExtractor::apply(*_db, iter));
+    }
+}
+
+template <typename DataStoreTraitsT>
 uint64_t GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::generation() const noexcept {
     return _guard.getGeneration();
 }
diff --git a/storage/src/vespa/storage/bucketdb/lockablemap.h b/storage/src/vespa/storage/bucketdb/lockablemap.h
index 66619a4f7e8..a79a42b44ab 100644
--- a/storage/src/vespa/storage/bucketdb/lockablemap.h
+++ b/storage/src/vespa/storage/bucketdb/lockablemap.h
@@ -124,11 +124,13 @@ private:
         WaiterMap _map;
     };
 
-    Map               _map;
+    class ReadGuardImpl;
+
+    Map                     _map;
     mutable std::mutex      _lock;
     std::condition_variable _cond;
-    LockIdSet         _lockedKeys;
-    LockWaiters       _lockWaiters;
+    LockIdSet               _lockedKeys;
+    LockWaiters             _lockWaiters;
 
     void unlock(const key_type& key) override;
     bool findNextKey(key_type& key, mapped_type& val, const char* clientId,
@@ -146,11 +148,13 @@ private:
                      const key_type& first,
                      const key_type& last) override;
 
-    void do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func,
+    void do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func,
                              const char* clientId,
                              vespalib::duration yieldTime,
                              uint32_t chunkSize) override;
 
+    std::unique_ptr<bucketdb::ReadGuard<typename Map::mapped_type>> do_acquire_read_guard() const override;
+
     /**
      * Process up to `chunkSize` bucket database entries from--and possibly
      * including--the bucket pointed to by `key`.
@@ -162,7 +166,7 @@ private:
      * Modifies `key` in-place to point to the next key to process for the next
      * invocation of this function.
      */
-    bool processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func,
+    bool processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func,
                           key_type& key,
                           const char* clientId,
                           uint32_t chunkSize);
diff --git a/storage/src/vespa/storage/bucketdb/lockablemap.hpp b/storage/src/vespa/storage/bucketdb/lockablemap.hpp
index fcdde0a810c..c475361bd6f 100644
--- a/storage/src/vespa/storage/bucketdb/lockablemap.hpp
+++ b/storage/src/vespa/storage/bucketdb/lockablemap.hpp
@@ -247,7 +247,7 @@ void LockableMap<Map>::do_for_each(std::function<Decision(uint64_t, const mapped
 
 template <typename Map>
 bool
-LockableMap<Map>::processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func,
+LockableMap<Map>::processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func,
                                    key_type& key,
                                    const char* clientId,
                                    const uint32_t chunkSize)
@@ -268,7 +268,7 @@ LockableMap<Map>::processNextChunk(std::function<Decision(uint64_t, mapped_type&
 }
 
 template <typename Map>
-void LockableMap<Map>::do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func,
+void LockableMap<Map>::do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func,
                                            const char* clientId,
                                            vespalib::duration yieldTime,
                                            uint32_t chunkSize)
@@ -286,6 +286,55 @@ void LockableMap<Map>::do_for_each_chunked(std::function<Decision(uint64_t, mapp
     }
 }
 
+// TODO This is a placeholder that has to work around the const-ness and type quirks of
+// the legacy LockableMap implementation. In particular, it offers no snapshot isolation
+// at all, nor does it support the "get parents and self" bucket lookup operation.
+template <typename Map>
+class LockableMap<Map>::ReadGuardImpl final
+    : public bucketdb::ReadGuard<typename Map::mapped_type>
+{
+    const LockableMap<Map>& _map;
+public:
+    using mapped_type = typename Map::mapped_type;
+
+    explicit ReadGuardImpl(const LockableMap<Map>& map) : _map(map) {}
+    ~ReadGuardImpl() override = default;
+
+    std::vector<mapped_type> find_parents_and_self(const document::BucketId&) const override {
+        abort(); // Finding just parents+self isn't supported by underlying legacy DB API!
+    }
+
+    std::vector<mapped_type> find_parents_self_and_children(const document::BucketId& bucket) const override {
+        auto& mutable_map = const_cast<LockableMap<Map>&>(_map); // _map is thread safe.
+        auto locked_entries = mutable_map.getAll(bucket, "ReadGuardImpl::find_parents_self_and_children");
+        std::vector<mapped_type> entries;
+        entries.reserve(locked_entries.size());
+        for (auto& e : locked_entries) {
+            entries.emplace_back(*e.second);
+        }
+        return entries;
+    }
+
+    void for_each(std::function<void(uint64_t, const mapped_type&)> func) const override {
+        auto decision_wrapper = [&func](uint64_t key, const mapped_type& value) -> Decision {
+            func(key, value);
+            return Decision::CONTINUE;
+        };
+        auto& mutable_map = const_cast<LockableMap<Map>&>(_map); // _map is thread safe.
+        mutable_map.for_each_chunked(std::move(decision_wrapper), "ReadGuardImpl::for_each");
+    }
+
+    [[nodiscard]] uint64_t generation() const noexcept override {
+        return 0;
+    }
+};
+
+template <typename Map>
+std::unique_ptr<bucketdb::ReadGuard<typename Map::mapped_type>>
+LockableMap<Map>::do_acquire_read_guard() const {
+    return std::make_unique<ReadGuardImpl>(*this);
+}
+
 template<typename Map>
 void
 LockableMap<Map>::print(std::ostream& out, bool verbose,
diff --git a/storage/src/vespa/storage/bucketdb/read_guard.h b/storage/src/vespa/storage/bucketdb/read_guard.h
index cf37bafb0dd..bacb0a38e2f 100644
--- a/storage/src/vespa/storage/bucketdb/read_guard.h
+++ b/storage/src/vespa/storage/bucketdb/read_guard.h
@@ -2,6 +2,7 @@
 #pragma once
 
 #include <vespa/document/bucket/bucketid.h>
+#include <functional>
 #include <vector>
 
 namespace storage::bucketdb {
@@ -36,8 +37,9 @@ public:
     ReadGuard(const ReadGuard&) = delete;
     ReadGuard& operator=(const ReadGuard&) = delete;
 
-    virtual void find_parents_and_self(const document::BucketId& bucket,
-                                       std::vector<ValueT>& entries) const = 0;
+    virtual std::vector<ValueT> find_parents_and_self(const document::BucketId& bucket) const = 0;
+    virtual std::vector<ValueT> find_parents_self_and_children(const document::BucketId& bucket) const = 0;
+    virtual void for_each(std::function<void(uint64_t, const ValueT&)> func) const = 0;
     // If the underlying guard represents a snapshot, returns its monotonically
     // increasing generation. Otherwise returns 0.
     [[nodiscard]] virtual uint64_t generation() const noexcept = 0;
diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp
index 7066ea115fd..849164ace7d 100644
--- a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp
+++ b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp
@@ -136,6 +136,11 @@ void StorBucketDatabase::for_each(
     _impl->for_each(std::move(func), clientId, first, last);
 }
 
+std::unique_ptr<bucketdb::ReadGuard<StorBucketDatabase::Entry>>
+StorBucketDatabase::acquire_read_guard() const {
+    return _impl->acquire_read_guard();
+}
+
 template class JudyMultiMap<bucketdb::StorageBucketInfo>;
 
 } // storage
diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.h b/storage/src/vespa/storage/bucketdb/storbucketdb.h
index 87e5d80c01b..5aac27bccb0 100644
--- a/storage/src/vespa/storage/bucketdb/storbucketdb.h
+++ b/storage/src/vespa/storage/bucketdb/storbucketdb.h
@@ -2,6 +2,7 @@
 #pragma once
 
 #include "abstract_bucket_map.h"
+#include "read_guard.h"
 #include "storagebucketinfo.h"
 #include <vespa/storageapi/defs.h>
 #include <memory>
@@ -68,6 +69,8 @@ public:
                   const key_type& first = key_type(),
                   const key_type& last = key_type() - 1);
 
+    std::unique_ptr<bucketdb::ReadGuard<Entry>> acquire_read_guard() const;
+
     /**
      * Returns true iff bucket has no superbuckets or sub-buckets in the
      * database. Usage assumption is that any operation that can cause the
diff --git a/storage/src/vespa/storage/common/content_bucket_space_repo.h b/storage/src/vespa/storage/common/content_bucket_space_repo.h
index 142bb5ea1d5..6d8bd67b071 100644
--- a/storage/src/vespa/storage/common/content_bucket_space_repo.h
+++ b/storage/src/vespa/storage/common/content_bucket_space_repo.h
@@ -43,6 +43,13 @@ public:
         }
     }
 
+    template <typename Functor>
+    void for_each_bucket(Functor functor) const {
+        for (const auto& elem : _map) {
+            elem.second->bucketDatabase().acquire_read_guard()->for_each(std::move(functor));
+        }
+    }
+
 };
 
 }
diff --git a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp
index bbe477be9ef..c4fb9b8228c 100644
--- a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp
@@ -250,8 +250,7 @@ GetOperation::assignTargetNodeGroups(const BucketDatabase::ReadGuard& read_guard
     document::BucketIdFactory bucketIdFactory;
     document::BucketId bid = bucketIdFactory.getBucketId(_msg->getDocumentId());
 
-    std::vector<BucketDatabase::Entry> entries;
-    read_guard.find_parents_and_self(bid, entries);
+    auto entries = read_guard.find_parents_and_self(bid);
 
     for (uint32_t j = 0; j < entries.size(); ++j) {
         const BucketDatabase::Entry& e = entries[j];