Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh() is not supported by vector memtable which may lead to an error in db_stress #12286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion db/arena_wrapped_db_iter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,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);
}
Expand Down
1 change: 1 addition & 0 deletions db/arena_wrapped_db_iter.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,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;

Expand Down
23 changes: 23 additions & 0 deletions db/db_iterator_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3550,6 +3550,29 @@ TEST_F(DBIteratorTest, ErrorWhenReadFile) {
iter->Reset();
}

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) {
Expand Down
5 changes: 2 additions & 3 deletions db_stress_tool/no_batched_ops_stress.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1868,9 +1868,8 @@ class NonBatchedOpsStressTest : public StressTest {
op_logs += "P";
}

// Write-prepared and Write-unprepared do not support Refresh() yet.
if (!(FLAGS_use_txn && FLAGS_txn_write_policy != 0) &&
thread->rand.OneIn(2)) {
// Check if Refresh() supported.
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
Expand Down
5 changes: 4 additions & 1 deletion include/rocksdb/iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ class Iterator : public Cleanable {
virtual Status Refresh(const class Snapshot*) {
return Status::NotSupported("Refresh() is not supported");
}


// Check if iterator supports Refrsh()
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.
Expand Down
4 changes: 4 additions & 0 deletions include/rocksdb/memtablerep.h
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ class MemTableRepFactory : public Customizable {
// false when if the <key,seq> already exists.
// Default: false
virtual bool CanHandleDuplicatedKey() const { return false; }

// Return true if the memtable support Refresh()
virtual bool IsRefreshIterSupported() const { return true; }
};

// This uses a skip list to store keys. It is the default.
Expand Down Expand Up @@ -378,6 +381,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;
Expand Down