From c64f0b4f8280190525a6430388e714739f18976b Mon Sep 17 00:00:00 2001 From: Dan Burkert Date: Thu, 2 Aug 2018 23:43:06 -0700 Subject: [PATCH] Simplify empty database iter handling This commit changes the API of Cursor::iter_dup_of, and is thus a breaking change. --- src/cursor.rs | 99 ++++++++++++++++++++++----------------------------- 1 file changed, 42 insertions(+), 57 deletions(-) diff --git a/src/cursor.rs b/src/cursor.rs index 0baed51..f50eef4 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -1,6 +1,7 @@ -use libc::{c_void, size_t, c_uint}; -use std::{fmt, ptr, result, slice}; use std::marker::PhantomData; +use std::{fmt, mem, ptr, result, slice}; + +use libc::{EINVAL, c_void, size_t, c_uint}; use database::Database; use error::{Error, Result, lmdb_result}; @@ -19,11 +20,7 @@ pub trait Cursor<'txn> { /// Retrieves a key/data pair from the cursor. Depending on the cursor op, /// the current key may be returned. - fn get(&self, - key: Option<&[u8]>, - data: Option<&[u8]>, - op: c_uint) - -> Result<(Option<&'txn [u8]>, &'txn [u8])> { + fn get(&self, key: Option<&[u8]>, data: Option<&[u8]>, op: c_uint) -> Result<(Option<&'txn [u8]>, &'txn [u8])> { unsafe { let mut key_val = slice_to_val(key); let mut data_val = slice_to_val(data); @@ -52,14 +49,7 @@ pub trait Cursor<'txn> { /// duplicate data items of each key will be returned before moving on to /// the next key. fn iter_start(&mut self) -> Iter<'txn> { - // When the db is empty, this returns NotFound, which means the iterator should not even - // try to proceed or it too will error with code 22 - let complete = self.get(None, None, ffi::MDB_FIRST) - .map(|_| false) - .unwrap_or(true); - let mut iter = Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT); - iter.complete = complete; - iter + Iter::new(self.cursor(), ffi::MDB_FIRST, ffi::MDB_NEXT) } /// Iterate over database items starting from the given key. @@ -69,10 +59,9 @@ pub trait Cursor<'txn> { /// the next key. fn iter_from(&mut self, key: K) -> Iter<'txn> where K: AsRef<[u8]> { match self.get(Some(key.as_ref()), None, ffi::MDB_SET_RANGE) { - Err(Error::NotFound) => Ok(()), - Err(error) => Err(error), - Ok(_) => Ok(()), - }.unwrap(); + Ok(_) | Err(Error::NotFound) => (), + Err(error) => panic!("mdb_cursor_get returned an unexpected error: {}", error), + }; Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT) } @@ -86,27 +75,26 @@ pub trait Cursor<'txn> { /// Iterate over duplicate database items starting from the beginning of the /// database. Each item will be returned as an iterator of its duplicates. fn iter_dup_start(&mut self) -> IterDup<'txn> { - self.get(None, None, ffi::MDB_FIRST).unwrap(); - IterDup::new(self.cursor(), ffi::MDB_GET_CURRENT) + IterDup::new(self.cursor(), ffi::MDB_FIRST) } /// Iterate over duplicate items in the database starting from the given /// key. Each item will be returned as an iterator of its duplicates. fn iter_dup_from(&mut self, key: &K) -> IterDup<'txn> where K: AsRef<[u8]> { match self.get(Some(key.as_ref()), None, ffi::MDB_SET_RANGE) { - Err(Error::NotFound) => Ok(()), - Err(error) => Err(error), - Ok(_) => Ok(()), - }.unwrap(); + Ok(_) | Err(Error::NotFound) => (), + Err(error) => panic!("mdb_cursor_get returned an unexpected error: {}", error), + }; IterDup::new(self.cursor(), ffi::MDB_GET_CURRENT) } - /// Iterate over the duplicates of the item in the database with the given - /// key. - fn iter_dup_of(&mut self, key: &K) -> Result> where K: - AsRef<[u8]> { - self.get(Some(key.as_ref()), None, ffi::MDB_SET)?; - Ok(Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT_DUP)) + /// Iterate over the duplicates of the item in the database with the given key. + fn iter_dup_of(&mut self, key: &K) -> Iter<'txn> where K: AsRef<[u8]> { + match self.get(Some(key.as_ref()), None, ffi::MDB_SET) { + Ok(_) | Err(Error::NotFound) => (), + Err(error) => panic!("mdb_cursor_get returned an unexpected error: {}", error), + }; + Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT_DUP) } } @@ -231,7 +219,6 @@ pub struct Iter<'txn> { cursor: *mut ffi::MDB_cursor, op: c_uint, next_op: c_uint, - complete: bool, _marker: PhantomData, } @@ -239,7 +226,7 @@ impl <'txn> Iter<'txn> { /// Creates a new iterator backed by the given cursor. fn new<'t>(cursor: *mut ffi::MDB_cursor, op: c_uint, next_op: c_uint) -> Iter<'t> { - Iter { cursor: cursor, op: op, next_op: next_op, complete: false, _marker: PhantomData } + Iter { cursor: cursor, op: op, next_op: next_op, _marker: PhantomData } } } @@ -254,26 +241,16 @@ impl <'txn> Iterator for Iter<'txn> { type Item = (&'txn [u8], &'txn [u8]); fn next(&mut self) -> Option<(&'txn [u8], &'txn [u8])> { - if self.complete { - return None - } let mut key = ffi::MDB_val { mv_size: 0, mv_data: ptr::null_mut() }; let mut data = ffi::MDB_val { mv_size: 0, mv_data: ptr::null_mut() }; - + let op = mem::replace(&mut self.op, self.next_op); unsafe { - let err_code = ffi::mdb_cursor_get(self.cursor, &mut key, &mut data, self.op); - // Set the operation for the next get - self.op = self.next_op; - if err_code == ffi::MDB_SUCCESS { - Some((val_to_slice(key), val_to_slice(data))) - } else { - // The documentation for mdb_cursor_get specifies that it may fail with MDB_NOTFOUND - // and MDB_EINVAL (and we shouldn't be passing in invalid parameters). - // TODO: validate that these are the only failures possible. - debug_assert!(err_code == ffi::MDB_NOTFOUND, - "Unexpected LMDB error {:?}.", Error::from_err_code(err_code)); - self.complete = true; - None + match ffi::mdb_cursor_get(self.cursor, &mut key, &mut data, op) { + ffi::MDB_SUCCESS => Some((val_to_slice(key), val_to_slice(data))), + // EINVAL can occur when the cursor was previously seeked to a non-existent value, + // e.g. iter_from with a key greater than all values in the database. + ffi::MDB_NOTFOUND | EINVAL => None, + error => panic!("mdb_cursor_get returned an unexpected error: {}", error), } } } @@ -310,12 +287,12 @@ impl <'txn> Iterator for IterDup<'txn> { fn next(&mut self) -> Option> { let mut key = ffi::MDB_val { mv_size: 0, mv_data: ptr::null_mut() }; let mut data = ffi::MDB_val { mv_size: 0, mv_data: ptr::null_mut() }; + let op = mem::replace(&mut self.op, ffi::MDB_NEXT_NODUP); let err_code = unsafe { - ffi::mdb_cursor_get(self.cursor, &mut key, &mut data, self.op) + ffi::mdb_cursor_get(self.cursor, &mut key, &mut data, op) }; if err_code == ffi::MDB_SUCCESS { - self.op = ffi::MDB_NEXT; Some(Iter::new(self.cursor, ffi::MDB_GET_CURRENT, ffi::MDB_NEXT_DUP)) } else { None @@ -475,25 +452,33 @@ mod test { } #[test] - fn test_iter_start_empty() { + fn test_iter_empty_database() { let dir = TempDir::new("test").unwrap(); let env = Environment::new().open(dir.path()).unwrap(); let db = env.open_db(None).unwrap(); let txn = env.begin_ro_txn().unwrap(); let mut cursor = txn.open_ro_cursor(db).unwrap(); + assert_eq!(0, cursor.iter().count()); assert_eq!(0, cursor.iter_start().count()); + assert_eq!(0, cursor.iter_from(b"foo").count()); } #[test] - fn test_iter_empty() { + fn test_iter_empty_dup_database() { let dir = TempDir::new("test").unwrap(); let env = Environment::new().open(dir.path()).unwrap(); - let db = env.open_db(None).unwrap(); + let db = env.create_db(None, DatabaseFlags::DUP_SORT).unwrap(); let txn = env.begin_ro_txn().unwrap(); let mut cursor = txn.open_ro_cursor(db).unwrap(); assert_eq!(0, cursor.iter().count()); + assert_eq!(0, cursor.iter_start().count()); + assert_eq!(0, cursor.iter_from(b"foo").count()); + assert_eq!(0, cursor.iter_dup().count()); + assert_eq!(0, cursor.iter_dup_start().count()); + assert_eq!(0, cursor.iter_dup_from(b"foo").count()); + assert_eq!(0, cursor.iter_dup_of(b"foo").count()); } #[test] @@ -547,9 +532,9 @@ mod test { cursor.iter_dup_from(b"f").flat_map(|x| x).collect::>()); assert_eq!(items.clone().into_iter().skip(3).take(3).collect::>(), - cursor.iter_dup_of(b"b").unwrap().collect::>()); + cursor.iter_dup_of(b"b").collect::>()); - assert!(cursor.iter_dup_of(b"foo").is_err()); + assert_eq!(0, cursor.iter_dup_of(b"foo").count()); } #[test]