spacejam / sled

the champagne of beta embedded databases
Apache License 2.0
7.89k stars 377 forks source link

Transactional on a tuple of trees is not generic #908

Open wdullaer opened 4 years ago

wdullaer commented 4 years ago

Bug reports must include:

  1. sled version: 0.30.0
  2. rustc version: 1.39
  3. operating system: all
  4. logs: N/A

The macro implementation of Transactional on tuples of trees is not generic over the error: https://github.com/spacejam/sled/blob/master/src/transaction.rs#L537

The implementation for Tree is.

This means that I can't return an error with the abort helper function when running a transaction over a tuple of Trees like I can for a transaction on an individual Tree.

This compiles:

use sled::{TransactionError, TransactionResult, Config, abort};

#[derive(Debug, PartialEq)]
struct MyBullshitError;

fn main() -> TransactionResult<(), MyBullshitError> {
    let config = Config::new().temporary(true);
    let db = config.open().unwrap().open_tree("tree1").unwrap();

    // Use write-only transactions as a writebatch:
    let res = db.transaction(|db| {
        db.insert(b"k1", b"cats")?;
        db.insert(b"k2", b"dogs")?;
        // aborting will cause all writes to roll-back.
        if true {
            abort(MyBullshitError)?;
        }
        Ok(42)
    }).unwrap_err();

    assert_eq!(res, TransactionError::Abort(MyBullshitError));
    assert_eq!(db.get(b"k1")?, None);
    assert_eq!(db.get(b"k2")?, None);

    Ok(())
}

This does not compile:

use sled::{Transactional, TransactionError, TransactionResult, Config, abort};

#[derive(Debug, PartialEq)]
struct MyBullshitError;

fn main() -> TransactionResult<(), MyBullshitError> {
    let config = Config::new().temporary(true);
    let db = config.open().unwrap();
    let tree1 = db.open_tree("tree1").unwrap();
    let tree2 = db.open_tree("tree2").unwrap();

    // Use write-only transactions as a writebatch:
    let res = (tree1, tree2).transaction(|(tree1, tree2)| {
        tree1.insert(b"k1", b"cats")?;
        tree2.insert(b"k2", b"dogs")?;
        // aborting will cause all writes to roll-back.
        if true {
            abort(MyBullshitError)?;
        }
        Ok(42)
    }).unwrap_err();

    assert_eq!(res, TransactionError::Abort(MyBullshitError));
    assert_eq!(tree1.get(b"k1")?, None);
    assert_eq!(tree2.get(b"k2")?, None);

    Ok(())
}

Here's the output of rustc for that particular piece of code

error[E0277]: `?` couldn't convert the error to `sled::ConflictableTransactionError<()>`
  --> src/tree.rs:272:35
   |
20 |             abort(MyBullshitError)?;
   |                                   ^ the trait `std::convert::From<sled::ConflictableTransactionError<MyBullshitError>>` is not implemented for `sled::ConflictableTransactionError<()>`
   |

I tried fixing this, by changing line 537 to

        impl<E> Transactional<E> for repeat_type!(&Tree, ($($indices),+)) {

This would fix it, but it breaks type inference for closures that do not abort, which judging by https://github.com/spacejam/sled/issues/810#issuecomment-542082720 is undesirable. The example showing a transaction on tuple of trees gives a compiler errors that a type annotation is needed after this change is applied.

error[E0282]: type annotations needed
  --> src/tree.rs:308:6
   |
19 |     .transaction(|(unprocessed, processed)| {
   |      ^^^^^^^^^^^ cannot infer type for `E`

(I can't say I fully understand why type inference works for Transactable<E> on a Tree but not on a tuple of Trees)

NicolasDP commented 4 years ago

I believe this is because the impl of tuples for Transactional use tuples of TransactionalTree. All the operations on the TransactionalTree are UnabortableTransactionError.

So I think this is on purpose in order to warn the users that a transaction on multiple trees cannot be aborted.