From eb88d202fdb49432c4e6194ab47c1091f0969652 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Mon, 2 Dec 2019 14:30:59 +0100 Subject: [PATCH 1/5] kvdb-rocksdb: pass ReadOptions to iterators --- kvdb-rocksdb/src/iter.rs | 35 ++++++++++++++++++++++------------- kvdb-rocksdb/src/lib.rs | 8 +++++--- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index 52934e1a8..c273cb180 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -21,7 +21,7 @@ use crate::DBAndColumns; use owning_ref::{OwningHandle, StableAddress}; use parking_lot::RwLockReadGuard; -use rocksdb::{DBIterator, IteratorMode}; +use rocksdb::{DBIterator, Direction, IteratorMode, ReadOptions}; use std::ops::{Deref, DerefMut}; /// A tuple holding key and value data, used as the iterator item type. @@ -76,22 +76,27 @@ pub trait IterationHandler { /// Create an `Iterator` over the default DB column or over a `ColumnFamily` if a column number /// is passed. - fn iter(&self, col: Option) -> Self::Iterator; + fn iter(&self, col: Option, read_opts: &ReadOptions) -> Self::Iterator; /// Create an `Iterator` over the default DB column or over a `ColumnFamily` if a column number /// is passed. The iterator starts from the first key having the provided `prefix`. - fn iter_from_prefix(&self, col: Option, prefix: &[u8]) -> Self::Iterator; + fn iter_from_prefix(&self, col: Option, prefix: &[u8], read_opts: &ReadOptions) -> Self::Iterator; } impl<'a, T> ReadGuardedIterator<'a, <&'a T as IterationHandler>::Iterator, T> where &'a T: IterationHandler, { - pub fn new(read_lock: RwLockReadGuard<'a, Option>, col: Option) -> Self { - Self { inner: Self::new_inner(read_lock, |db| db.iter(col)) } + pub fn new(read_lock: RwLockReadGuard<'a, Option>, col: Option, read_opts: &ReadOptions) -> Self { + Self { inner: Self::new_inner(read_lock, |db| db.iter(col, read_opts)) } } - pub fn new_from_prefix(read_lock: RwLockReadGuard<'a, Option>, col: Option, prefix: &[u8]) -> Self { - Self { inner: Self::new_inner(read_lock, |db| db.iter_from_prefix(col, prefix)) } + pub fn new_from_prefix( + read_lock: RwLockReadGuard<'a, Option>, + col: Option, + prefix: &[u8], + read_opts: &ReadOptions, + ) -> Self { + Self { inner: Self::new_inner(read_lock, |db| db.iter_from_prefix(col, prefix, read_opts)) } } fn new_inner( @@ -108,21 +113,25 @@ where impl<'a> IterationHandler for &'a DBAndColumns { type Iterator = DBIterator<'a>; - fn iter(&self, col: Option) -> Self::Iterator { + fn iter(&self, col: Option, read_opts: &ReadOptions) -> Self::Iterator { col.map_or_else( - || self.db.iterator(IteratorMode::Start), + || self.db.iterator_opt(IteratorMode::Start, read_opts), |c| { self.db - .iterator_cf(self.get_cf(c as usize), IteratorMode::Start) + .iterator_cf_opt(self.get_cf(c as usize), read_opts, IteratorMode::Start) .expect("iterator params are valid; qed") }, ) } - fn iter_from_prefix(&self, col: Option, prefix: &[u8]) -> Self::Iterator { + fn iter_from_prefix(&self, col: Option, prefix: &[u8], read_opts: &ReadOptions) -> Self::Iterator { col.map_or_else( - || self.db.prefix_iterator(prefix), - |c| self.db.prefix_iterator_cf(self.get_cf(c as usize), prefix).expect("iterator params are valid; qed"), + || self.db.iterator_opt(IteratorMode::From(prefix, Direction::Forward), read_opts), + |c| { + self.db + .iterator_cf_opt(self.get_cf(c as usize), read_opts, IteratorMode::From(prefix, Direction::Forward)) + .expect("iterator params are valid; qed") + }, ) } } diff --git a/kvdb-rocksdb/src/lib.rs b/kvdb-rocksdb/src/lib.rs index 3a0905273..e0596e082 100644 --- a/kvdb-rocksdb/src/lib.rs +++ b/kvdb-rocksdb/src/lib.rs @@ -571,7 +571,7 @@ impl Database { overlay_data }; - let guarded = iter::ReadGuardedIterator::new(read_lock, col); + let guarded = iter::ReadGuardedIterator::new(read_lock, col, &self.read_opts); Some(interleave_ordered(overlay_data, guarded)) } else { None @@ -588,12 +588,14 @@ impl Database { ) -> impl Iterator + 'a { let read_lock = self.db.read(); let optional = if read_lock.is_some() { - let guarded = iter::ReadGuardedIterator::new_from_prefix(read_lock, col, prefix); + let guarded = iter::ReadGuardedIterator::new_from_prefix(read_lock, col, prefix, &self.read_opts); Some(interleave_ordered(Vec::new(), guarded)) } else { None }; - // workaround for https://github.com/facebook/rocksdb/issues/2343 + // We're not using "Prefix Seek" mode, so the iterator will return + // keys not starting with the given prefix as well, + // see https://github.com/facebook/rocksdb/wiki/Prefix-Seek-API-Changes optional.into_iter().flat_map(identity).filter(move |(k, _)| k.starts_with(prefix)) } From 18811bc1e0f7081fb439d241eb1e78eeb05c33b3 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Wed, 4 Dec 2019 16:38:09 +0100 Subject: [PATCH 2/5] kvdb-rocksdb: add some iter docs --- kvdb-rocksdb/src/iter.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index c273cb180..a7c6c81dd 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -17,6 +17,10 @@ //! This module contains an implementation of a RocksDB iterator //! wrapped inside a `RwLock`. Since `RwLock` "owns" the inner data, //! we're using `owning_ref` to work around the borrowing rules of Rust. +//! +//! Note, that we're not using "Prefix Seek" mode, so that the prefix iterator will return +//! keys not starting with the given prefix as well, and we need to filter them +//! out manually. See https://github.com/facebook/rocksdb/wiki/Prefix-Seek-API-Changes use crate::DBAndColumns; use owning_ref::{OwningHandle, StableAddress}; @@ -86,10 +90,14 @@ impl<'a, T> ReadGuardedIterator<'a, <&'a T as IterationHandler>::Iterator, T> where &'a T: IterationHandler, { + /// Maps `RwLock` to `RwLock`, where + /// `DBIterator` iterates over all keys. pub fn new(read_lock: RwLockReadGuard<'a, Option>, col: Option, read_opts: &ReadOptions) -> Self { Self { inner: Self::new_inner(read_lock, |db| db.iter(col, read_opts)) } } + /// Maps `RwLock` to `RwLock`, where + /// `DBIterator` iterates over keys >= prefix. pub fn new_from_prefix( read_lock: RwLockReadGuard<'a, Option>, col: Option, From b428c3c01be43b7b4058210c0d6c4e989984020c Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 5 Dec 2019 11:07:47 +0100 Subject: [PATCH 3/5] Apply David's suggestions from code review Co-Authored-By: David --- kvdb-rocksdb/src/iter.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index a7c6c81dd..4f5b7e0f3 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -18,9 +18,9 @@ //! wrapped inside a `RwLock`. Since `RwLock` "owns" the inner data, //! we're using `owning_ref` to work around the borrowing rules of Rust. //! -//! Note, that we're not using "Prefix Seek" mode, so that the prefix iterator will return -//! keys not starting with the given prefix as well, and we need to filter them -//! out manually. See https://github.com/facebook/rocksdb/wiki/Prefix-Seek-API-Changes +//! Note: this crate does not use "Prefix Seek" mode which means that the prefix iterator will return +//! keys not starting with the given prefix as well (as long as `key >= prefix`). To work around this we filter the data returned by rocksdb to ensure that all data yielded by the iterator does start with the given prefix. +//! See https://github.com/facebook/rocksdb/wiki/Prefix-Seek-API-Changes for details. use crate::DBAndColumns; use owning_ref::{OwningHandle, StableAddress}; @@ -90,8 +90,9 @@ impl<'a, T> ReadGuardedIterator<'a, <&'a T as IterationHandler>::Iterator, T> where &'a T: IterationHandler, { - /// Maps `RwLock` to `RwLock`, where + /// Creates a new `ReadGuardedIterator` that maps `RwLock` to `RwLock`, where /// `DBIterator` iterates over all keys. + /// In addition to a read lock and a column index, it takes a ref to `ReadOptions` to allow customization of the iterator behaviour, e.g. . pub fn new(read_lock: RwLockReadGuard<'a, Option>, col: Option, read_opts: &ReadOptions) -> Self { Self { inner: Self::new_inner(read_lock, |db| db.iter(col, read_opts)) } } From 429a02dd604bdc4310f5569eaeafb850c1b90409 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 5 Dec 2019 11:23:25 +0100 Subject: [PATCH 4/5] kvdb-rocksdb: postprocess docs --- kvdb-rocksdb/src/iter.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index c4ed46096..466a2d31c 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -18,8 +18,10 @@ //! wrapped inside a `RwLock`. Since `RwLock` "owns" the inner data, //! we're using `owning_ref` to work around the borrowing rules of Rust. //! -//! Note: this crate does not use "Prefix Seek" mode which means that the prefix iterator will return -//! keys not starting with the given prefix as well (as long as `key >= prefix`). To work around this we filter the data returned by rocksdb to ensure that all data yielded by the iterator does start with the given prefix. +//! Note: this crate does not use "Prefix Seek" mode which means that the prefix iterator +//! will return keys not starting with the given prefix as well (as long as `key >= prefix`). +//! To work around this we filter the data returned by rocksdb to ensure that +//! all data yielded by the iterator does start with the given prefix. //! See https://github.com/facebook/rocksdb/wiki/Prefix-Seek-API-Changes for details. use crate::DBAndColumns; @@ -80,9 +82,13 @@ pub trait IterationHandler { /// Create an `Iterator` over the default DB column or over a `ColumnFamily` if a column number /// is passed. + /// In addition to a read lock and a column index, it takes a ref to the same `ReadOptions` we + /// pass to the `get` method. fn iter(&self, col: Option, read_opts: &ReadOptions) -> Self::Iterator; /// Create an `Iterator` over the default DB column or over a `ColumnFamily` if a column number /// is passed. The iterator starts from the first key having the provided `prefix`. + /// In addition to a read lock and a column index, it takes a ref to the same `ReadOptions` we + /// pass to the `get` method. fn iter_from_prefix(&self, col: Option, prefix: &[u8], read_opts: &ReadOptions) -> Self::Iterator; } @@ -90,15 +96,14 @@ impl<'a, T> ReadGuardedIterator<'a, <&'a T as IterationHandler>::Iterator, T> where &'a T: IterationHandler, { - /// Creates a new `ReadGuardedIterator` that maps `RwLock` to `RwLock`, where - /// `DBIterator` iterates over all keys. - /// In addition to a read lock and a column index, it takes a ref to `ReadOptions` to allow customization of the iterator behaviour, e.g. . + /// Creates a new `ReadGuardedIterator` that maps `RwLock` to `RwLock`, + /// where `DBIterator` iterates over all keys. pub fn new(read_lock: RwLockReadGuard<'a, Option>, col: Option, read_opts: &ReadOptions) -> Self { Self { inner: Self::new_inner(read_lock, |db| db.iter(col, read_opts)) } } - /// Maps `RwLock` to `RwLock`, where - /// `DBIterator` iterates over keys >= prefix. + /// Creates a new `ReadGuardedIterator` that maps `RwLock` to `RwLock`, + /// where `DBIterator` iterates over keys >= prefix. pub fn new_from_prefix( read_lock: RwLockReadGuard<'a, Option>, col: Option, From 9c5987114fe045a0efd3cf438544a8fe85b83ecb Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 17 Dec 2019 19:28:35 +0100 Subject: [PATCH 5/5] formatting --- kvdb-rocksdb/src/iter.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index 9091a2c14..4ea9a9e92 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -130,10 +130,14 @@ impl<'a> IterationHandler for &'a DBAndColumns { type Iterator = DBIterator<'a>; fn iter(&self, col: u32, read_opts: &ReadOptions) -> Self::Iterator { - self.db.iterator_cf_opt(self.cf(col as usize), read_opts, IteratorMode::Start).expect("iterator params are valid; qed") - } + self.db + .iterator_cf_opt(self.cf(col as usize), read_opts, IteratorMode::Start) + .expect("iterator params are valid; qed") + } fn iter_from_prefix(&self, col: u32, prefix: &[u8], read_opts: &ReadOptions) -> Self::Iterator { - self.db.iterator_cf_opt(self.cf(col as usize), read_opts, IteratorMode::From(prefix, Direction::Forward)).expect("iterator params are valid; qed") + self.db + .iterator_cf_opt(self.cf(col as usize), read_opts, IteratorMode::From(prefix, Direction::Forward)) + .expect("iterator params are valid; qed") } }