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

storage: fixes behaviour of cursor delete_current on start item #7646

Merged
merged 3 commits into from Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions crates/storage/db/src/abstraction/cursor.rs
Expand Up @@ -158,7 +158,7 @@ impl<'cursor, T: Table, CURSOR: DbCursorRO<T>> Iterator for Walker<'cursor, T, C
fn next(&mut self) -> Option<Self::Item> {
let start = self.start.take();
if start.is_some() {
return start
return start;
rkrasiuk marked this conversation as resolved.
Show resolved Hide resolved
}

self.cursor.next().transpose()
Expand All @@ -181,6 +181,7 @@ impl<'cursor, T: Table, CURSOR: DbCursorRO<T>> Walker<'cursor, T, CURSOR> {
impl<'cursor, T: Table, CURSOR: DbCursorRW<T> + DbCursorRO<T>> Walker<'cursor, T, CURSOR> {
/// Delete current item that walker points to.
pub fn delete_current(&mut self) -> Result<(), DatabaseError> {
self.start.take();
self.cursor.delete_current()
}
}
Expand Down Expand Up @@ -223,6 +224,7 @@ impl<'cursor, T: Table, CURSOR: DbCursorRO<T>> ReverseWalker<'cursor, T, CURSOR>
impl<'cursor, T: Table, CURSOR: DbCursorRW<T> + DbCursorRO<T>> ReverseWalker<'cursor, T, CURSOR> {
/// Delete current item that walker points to.
pub fn delete_current(&mut self) -> Result<(), DatabaseError> {
self.start.take();
self.cursor.delete_current()
}
}
Expand All @@ -233,7 +235,7 @@ impl<'cursor, T: Table, CURSOR: DbCursorRO<T>> Iterator for ReverseWalker<'curso
fn next(&mut self) -> Option<Self::Item> {
let start = self.start.take();
if start.is_some() {
return start
return start;
rkrasiuk marked this conversation as resolved.
Show resolved Hide resolved
}

self.cursor.prev().transpose()
Expand Down Expand Up @@ -273,7 +275,7 @@ impl<'cursor, T: Table, CURSOR: DbCursorRO<T>> Iterator for RangeWalker<'cursor,

fn next(&mut self) -> Option<Self::Item> {
if self.is_done {
return None
return None;
rkrasiuk marked this conversation as resolved.
Show resolved Hide resolved
}

let next_item = self.start.take().or_else(|| self.cursor.next().transpose());
Expand Down Expand Up @@ -321,6 +323,7 @@ impl<'cursor, T: Table, CURSOR: DbCursorRO<T>> RangeWalker<'cursor, T, CURSOR> {
impl<'cursor, T: Table, CURSOR: DbCursorRW<T> + DbCursorRO<T>> RangeWalker<'cursor, T, CURSOR> {
/// Delete current item that walker points to.
pub fn delete_current(&mut self) -> Result<(), DatabaseError> {
self.start.take();
self.cursor.delete_current()
}
}
Expand Down Expand Up @@ -353,6 +356,7 @@ where
impl<'cursor, T: DupSort, CURSOR: DbCursorRW<T> + DbDupCursorRO<T>> DupWalker<'cursor, T, CURSOR> {
/// Delete current item that walker points to.
pub fn delete_current(&mut self) -> Result<(), DatabaseError> {
self.start.take();
self.cursor.delete_current()
}
}
Expand All @@ -362,7 +366,7 @@ impl<'cursor, T: DupSort, CURSOR: DbDupCursorRO<T>> Iterator for DupWalker<'curs
fn next(&mut self) -> Option<Self::Item> {
let start = self.start.take();
if start.is_some() {
return start
return start;
rkrasiuk marked this conversation as resolved.
Show resolved Hide resolved
}
self.cursor.next_dup().transpose()
}
Expand Down
44 changes: 42 additions & 2 deletions crates/storage/db/src/implementation/mdbx/mod.rs
Expand Up @@ -367,7 +367,7 @@ impl DatabaseEnv {
LogLevel::Extra => 7,
});
} else {
return Err(DatabaseError::LogLevelUnavailable(log_level))
return Err(DatabaseError::LogLevelUnavailable(log_level));
rkrasiuk marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -411,7 +411,7 @@ impl DatabaseEnv {
/// Records version that accesses the database with write privileges.
pub fn record_client_version(&self, version: ClientVersion) -> Result<(), DatabaseError> {
if version.is_empty() {
return Ok(())
return Ok(());
rkrasiuk marked this conversation as resolved.
Show resolved Hide resolved
}

let tx = self.tx_mut()?;
Expand Down Expand Up @@ -478,6 +478,7 @@ mod tests {
const ERROR_APPEND: &str = "Not able to append the value to the table.";
const ERROR_UPSERT: &str = "Not able to upsert the value to the table.";
const ERROR_GET: &str = "Not able to get value from table.";
const ERROR_DEL: &str = "Not able to delete from table.";
const ERROR_COMMIT: &str = "Not able to commit transaction.";
const ERROR_RETURN_VALUE: &str = "Mismatching result.";
const ERROR_INIT_TX: &str = "Failed to create a MDBX transaction.";
Expand Down Expand Up @@ -507,6 +508,45 @@ mod tests {
tx.commit().expect(ERROR_COMMIT);
}

#[test]
fn db_dup_cursor_delete_first() {
let db: Arc<DatabaseEnv> = create_test_db(DatabaseEnvKind::RW);
let tx = db.tx_mut().expect(ERROR_INIT_TX);

let mut dup_cursor = tx.cursor_dup_write::<PlainStorageState>().unwrap();

let entry_0 = StorageEntry { key: B256::with_last_byte(1), value: U256::from(0) };
let entry_1 = StorageEntry { key: B256::with_last_byte(1), value: U256::from(1) };

dup_cursor.upsert(Address::with_last_byte(1), entry_0).expect(ERROR_UPSERT);
dup_cursor.upsert(Address::with_last_byte(1), entry_1).expect(ERROR_UPSERT);

assert_eq!(
dup_cursor.walk(None).unwrap().collect::<Result<Vec<_>, _>>(),
Ok(vec![(Address::with_last_byte(1), entry_0), (Address::with_last_byte(1), entry_1),])
);

let mut walker = dup_cursor.walk(None).unwrap();
walker.delete_current().expect(ERROR_DEL);

assert_eq!(walker.next(), Some(Ok((Address::with_last_byte(1), entry_1))));

// Check the tx view - it correctly holds entry_1
assert_eq!(
tx.cursor_dup_read::<PlainStorageState>()
.unwrap()
.walk(None)
.unwrap()
.collect::<Result<Vec<_>, _>>(),
Ok(vec![
(Address::with_last_byte(1), entry_1), // This is ok - we removed entry_0
])
);

// Check the remainder of walker
assert_eq!(walker.next(), None);
}

#[test]
fn db_cursor_walk() {
let env = create_test_db(DatabaseEnvKind::RW);
Expand Down