Refresh exist iterator isnot supported in hash spdb and vector memtab…

…lerep should avoid that (speedb-io#802) (speedb-io#811)

* Refresh exist iterator isnot supported in hash spdb and vector memtablerep should avoid that (speedb-io#802)

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

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);
   }

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;
 

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) {

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<int64_t>(expected_values_size); ++i) {
         post_read_expected_values.push_back(
             shared->Get(rand_column_family, i + lb));

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.

include/rocksdb/memtablerep.h

@@ -378,6 +378,7 @@ class MemTableRepFactory : public Customizable {
   // false when if the <key,seq> 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;

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,

monitoring/statistics.cc

@@ -389,8 +389,6 @@ static std::unordered_map<std::string, OptionTypeInfo> stats_type_info = {
 StatisticsImpl::StatisticsImpl(std::shared_ptr<Statistics> 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() {}

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,