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

Panic when deleting a table that was opened #715

Closed
Frando opened this issue Nov 7, 2023 · 2 comments · Fixed by #716
Closed

Panic when deleting a table that was opened #715

Frando opened this issue Nov 7, 2023 · 2 comments · Fixed by #716

Comments

@Frando
Copy link

Frando commented Nov 7, 2023

Hi,
when deleting a table in a transaction where the table was also opened, redb panics with

panicked at 'called `Option::unwrap()` on a `None` value', [...]/redb-1.3.0/src/tree_store/table_tree.rs:527:65

Example:

use redb::{Database, TableDefinition};

const T1: TableDefinition<u64, u64> = TableDefinition::new("t1");

fn main() -> anyhow::Result<()> {
    let db = Database::create("./foo.db")?;
    let tx = db.begin_write()?;
    {
        let _t1 = tx.open_table(T1)?;
        tx.delete_table(T1)?;
    }
    tx.commit()?;
    Ok(())
}

I was quite surprised by this. Can this be made into an error instead of panicking? We'd quite like redb to not panic. And if it is expected behavior, should it be added to the docs that WriteTransaction::delete_table may not be called on tables that are opened in the same transaction.

In iroh we have a database migration that moves values from a table-old into a table-new, and then deletes table-old. It looks roughly like this, and we'd preferably run this in a single transaction for idempotency. Am I missing something or is this not possible?

let tx = db.begin_write()?;
{
    let old_table_exists = tx
        .list_tables()?
        .any(|handle| handle.name() == TABLE_OLD.name());
    if !old_table_exists {
        return Ok(());
    }

    let table_old = tx.open_table(TABLE_OLD)?;
    let mut table_new = tx.open_table(TABLE_NEW)?;
    let iter = table_old.iter()?;
    for row in iter {
        let (k, v) = row?;
        table_new.insert(k.value(), migrate_value(v.value())?;
    }
    tx.delete_table(TABLE_OLD)?;
}
tx.commit()?;
@Frando
Copy link
Author

Frando commented Nov 7, 2023

Wow, this was fast! Thank you 🙏

@cberner
Copy link
Owner

cberner commented Nov 7, 2023

For sure!

github-merge-queue bot pushed a commit to n0-computer/iroh that referenced this issue Nov 7, 2023
## Description

The migration as commited with #1770 panics when starting iroh with an
existing data dir. The reason is that redb panics when deleting table in
a transaction where they were opened as well. See:
cberner/redb#715

This fixes this by moving the table deletion to a separate transaction.

The limitation and potential panic in redb was fixed in
cberner/redb#716 so once we upgrade to the next
(still unreleased) version of redb, this can be removed again.

## Change checklist

- [x] Self-review.
- [x] Documentation updates if relevant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants