From 5fd550e2440ec9d9117ca4d7d2a693cffe6a4df7 Mon Sep 17 00:00:00 2001 From: ayulas <107927247+ayulas@users.noreply.github.com> Date: Mon, 22 Jan 2024 16:03:43 +0200 Subject: [PATCH] Refresh exist iterator isnot supported in hash spdb and vector memtablerep should avoid that (#802) (#811) * Refresh exist iterator isnot supported in hash spdb and vector memtablerep should avoid that (#802) --- HISTORY.md | 5 ++- db/arena_wrapped_db_iter.cc | 5 ++- db/arena_wrapped_db_iter.h | 15 ++++++++ db/db_iterator_test.cc | 48 +++++++++++++++++++++++++ db_stress_tool/no_batched_ops_stress.cc | 6 +++- include/rocksdb/iterator.h | 17 ++++++++- include/rocksdb/memtablerep.h | 2 ++ memtable/hash_spdb_rep.cc | 1 + monitoring/statistics.cc | 2 -- tools/db_crashtest.py | 7 ++-- 10 files changed, 97 insertions(+), 11 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index e9a560f578..db2af7c1bc 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -16,6 +16,10 @@ Based on RocksDB 8.6.7 * LOG Enhancement: Have a separate LOG entry per CF Stats. This ensures that no CF stats data is lost in case the size of the combined CF stats text exceeds the LOG's threshold (#534). ### Bug Fixes +* Added IsRefreshIterSupported() to memtable_rep, to publish if the memtable support Refresh() of the iterator. +Refresh() will return status NotSupported for memtables that do not support Refresh(). +IsAllowRefresh() has been added. +db_stress has been updated as well to take into account that some memtables do not support Refresh() * fix conflicts between db_bench flags and enable speedb features flag(#743). * Proactive Flushes: Fix a race in the ShouldInitiateAnotherFlushMemOnly that may cause the method to return an incorrect answer (#758). * Stall deadlock consists small cfs (#637). @@ -29,7 +33,6 @@ RocksDB has a value of 10 by default and we've added the option to randomize the * Remove leftover references to ROCKSDB_LITE (#755). * Options: Set compaction_readahead_size default to 0. The current default of 2Mb is not optimal for most of our use cases. Having a value of 0 means that the FS will use its default size for prefetching (true only with https://github.com/speedb-io/speedb/pull/788). * Options: Set level_compaction_dynamic_level_bytes as false by default. This flag is not working properly with Speedb. see https://github.com/speedb-io/speedb/issues/786 for more details. -* stress test: Disable hash speedb memtable and enable_speedb_features from testing until issues are solved. ## Hazlenut 2.7.0 (27/10/2023) Based on RocksDB 8.1.1 diff --git a/db/arena_wrapped_db_iter.cc b/db/arena_wrapped_db_iter.cc index 5f7fed6d1a..8a8f79c1c5 100644 --- a/db/arena_wrapped_db_iter.cc +++ b/db/arena_wrapped_db_iter.cc @@ -173,7 +173,10 @@ ArenaWrappedDBIter* NewArenaWrappedDbIterator( ArenaWrappedDBIter* iter = new ArenaWrappedDBIter(); iter->Init(env, read_options, ioptions, mutable_cf_options, version, sequence, max_sequential_skip_in_iterations, version_number, read_callback, - db_impl, cfd, expose_blob_index, allow_refresh); + db_impl, cfd, expose_blob_index, + !ioptions.memtable_factory->IsRefreshIterSupported() + ? false + : allow_refresh); if (db_impl != nullptr && cfd != nullptr && allow_refresh) { iter->StoreRefreshInfo(db_impl, cfd, read_callback, expose_blob_index); } diff --git a/db/arena_wrapped_db_iter.h b/db/arena_wrapped_db_iter.h index f15be306d2..f0dd706157 100644 --- a/db/arena_wrapped_db_iter.h +++ b/db/arena_wrapped_db_iter.h @@ -1,3 +1,17 @@ +// Copyright (C) 2023 Speedb Ltd. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + // Copyright (c) 2011-present, Facebook, Inc. All rights reserved. // This source code is licensed under both the GPLv2 (found in the // COPYING file in the root directory) and Apache 2.0 License @@ -76,6 +90,7 @@ class ArenaWrappedDBIter : public Iterator { Status status() const override { return db_iter_->status(); } Slice timestamp() const override { return db_iter_->timestamp(); } bool IsBlob() const { return db_iter_->IsBlob(); } + bool IsAllowRefresh() override { return allow_refresh_; } Status GetProperty(std::string prop_name, std::string* prop) override; diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index 862377b6db..2a3967d6ad 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -1,3 +1,17 @@ +// Copyright (C) 2023 Speedb Ltd. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + // Copyright (c) 2011-present, Facebook, Inc. All rights reserved. // This source code is licensed under both the GPLv2 (found in the // COPYING file in the root directory) and Apache 2.0 License @@ -3295,6 +3309,40 @@ TEST_F(DBIteratorTest, IteratorRefreshReturnSV) { Close(); } +TEST_F(DBIteratorTest, HashSpdbRefreshStatus) { + Options options = CurrentOptions(); + options.memtable_factory.reset(NewHashSpdbRepFactory()); + DestroyAndReopen(options); + Iterator* iter = db_->NewIterator(ReadOptions()); + Status s = iter->Refresh(); + ASSERT_TRUE(s.IsNotSupported()); + ASSERT_FALSE(iter->IsAllowRefresh()); + delete iter; +} + +TEST_F(DBIteratorTest, VectorRefreshStatus) { + Options options = CurrentOptions(); + options.allow_concurrent_memtable_write = false; + options.memtable_factory.reset(new VectorRepFactory()); + DestroyAndReopen(options); + Iterator* iter = db_->NewIterator(ReadOptions()); + Status s = iter->Refresh(); + ASSERT_TRUE(s.IsNotSupported()); + ASSERT_FALSE(iter->IsAllowRefresh()); + delete iter; +} + +TEST_F(DBIteratorTest, SkipListRefreshStatus) { + Options options = CurrentOptions(); + options.memtable_factory.reset(new SkipListFactory()); + DestroyAndReopen(options); + Iterator* iter = db_->NewIterator(ReadOptions()); + Status s = iter->Refresh(); + ASSERT_OK(s); + ASSERT_TRUE(iter->IsAllowRefresh()); + delete iter; +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/db_stress_tool/no_batched_ops_stress.cc b/db_stress_tool/no_batched_ops_stress.cc index 57d5fb8a75..442ed84d5c 100644 --- a/db_stress_tool/no_batched_ops_stress.cc +++ b/db_stress_tool/no_batched_ops_stress.cc @@ -1844,7 +1844,7 @@ class NonBatchedOpsStressTest : public StressTest { op_logs += "P"; } - if (thread->rand.OneIn(2)) { + if (thread->rand.OneIn(2) && iter->IsAllowRefresh()) { pre_read_expected_values.clear(); post_read_expected_values.clear(); // Refresh after forward/backward scan to allow higher chance of SV @@ -1853,7 +1853,11 @@ class NonBatchedOpsStressTest : public StressTest { pre_read_expected_values.push_back( shared->Get(rand_column_family, i + lb)); } + // the return of Refresh doesnt has effect here cause we clear the + // pre/post expected values before. thats why we add the previous check of + // IsAllowRefresh iter->Refresh(); + for (int64_t i = 0; i < static_cast(expected_values_size); ++i) { post_read_expected_values.push_back( shared->Get(rand_column_family, i + lb)); diff --git a/include/rocksdb/iterator.h b/include/rocksdb/iterator.h index 9d4c9f73a1..937200b0be 100644 --- a/include/rocksdb/iterator.h +++ b/include/rocksdb/iterator.h @@ -1,3 +1,17 @@ +// Copyright (C) 2023 Speedb Ltd. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + // Copyright (c) 2011-present, Facebook, Inc. All rights reserved. // This source code is licensed under both the GPLv2 (found in the // COPYING file in the root directory) and Apache 2.0 License @@ -113,7 +127,8 @@ class Iterator : public Cleanable { virtual Status Refresh() { return Status::NotSupported("Refresh() is not supported"); } - + // Internally - previous check to Refresh + virtual bool IsAllowRefresh() { return true; } // Property "rocksdb.iterator.is-key-pinned": // If returning "1", this means that the Slice returned by key() is valid // as long as the iterator is not deleted. diff --git a/include/rocksdb/memtablerep.h b/include/rocksdb/memtablerep.h index 6344def611..6bb8baf190 100644 --- a/include/rocksdb/memtablerep.h +++ b/include/rocksdb/memtablerep.h @@ -378,6 +378,7 @@ class MemTableRepFactory : public Customizable { // false when if the already exists. // Default: false virtual bool CanHandleDuplicatedKey() const { return false; } + virtual bool IsRefreshIterSupported() const { return true; } virtual MemTableRep* PreCreateMemTableRep() { return nullptr; } virtual void PostCreateMemTableRep( MemTableRep* /*switch_mem*/, @@ -495,6 +496,7 @@ class VectorRepFactory : public MemTableRepFactory { static const char* kNickName() { return "vector"; } const char* Name() const override { return kClassName(); } const char* NickName() const override { return kNickName(); } + bool IsRefreshIterSupported() const override { return false; } // Methods for MemTableRepFactory class overrides using MemTableRepFactory::CreateMemTableRep; diff --git a/memtable/hash_spdb_rep.cc b/memtable/hash_spdb_rep.cc index 5263770852..44858746da 100644 --- a/memtable/hash_spdb_rep.cc +++ b/memtable/hash_spdb_rep.cc @@ -568,6 +568,7 @@ class HashSpdbRepFactory : public MemTableRepFactory { Logger* logger) override; bool IsInsertConcurrentlySupported() const override { return true; } bool CanHandleDuplicatedKey() const override { return true; } + bool IsRefreshIterSupported() const override { return false; } MemTableRep* PreCreateMemTableRep() override; void PostCreateMemTableRep(MemTableRep* switch_mem, const MemTableRep::KeyComparator& compare, diff --git a/monitoring/statistics.cc b/monitoring/statistics.cc index 6b9ca4b6c9..b6a19badd0 100644 --- a/monitoring/statistics.cc +++ b/monitoring/statistics.cc @@ -389,8 +389,6 @@ static std::unordered_map stats_type_info = { StatisticsImpl::StatisticsImpl(std::shared_ptr stats) : stats_(std::move(stats)) { RegisterOptions("StatisticsOptions", &stats_, &stats_type_info); - printf("StatisticsData.size=%d\n", (int)sizeof(StatisticsData)); - printf("per_core_stats_.size=%d\n", (int)sizeof(per_core_stats_)); } StatisticsImpl::~StatisticsImpl() {} diff --git a/tools/db_crashtest.py b/tools/db_crashtest.py index 4f71f1ae6c..44f56d1dec 100644 --- a/tools/db_crashtest.py +++ b/tools/db_crashtest.py @@ -247,15 +247,12 @@ "sync_wal_one_in": 100000, "customopspercent": 0, # "filter_uri": lambda: random.choice(["speedb.PairedBloomFilter", ""]), - # disable hash_spd until issues are fixed. - # "memtablerep": lambda: random.choice(["skip_list", "hash_spdb"]), - "memtablerep": "skip_list", + "memtablerep": lambda: random.choice(["skip_list", "hash_spdb"]), "pinning_policy": lambda: random.choice(["default", "scoped"]), "use_dynamic_delay": lambda: random.choice([0, 1, 1, 1]), "allow_wbm_stalls": lambda: random.randint(0, 1), "start_delay_percent": lambda: random.randint(0, 99), - # disable until hash_spd issues are fixed. - # "enable_speedb_features": lambda: random.randint(0, 1), + "enable_speedb_features": lambda: random.randint(0, 1), "total_ram_size": lambda: random.choice([512 * 1024 * 1024, 1024 * 1024 * 1024]), "max_background_jobs": lambda: random.choice([4, 8]), "crash_test": 1,