cberner / redb

An embedded key-value database in pure Rust
https://www.redb.org
Apache License 2.0
3.22k stars 147 forks source link

Represent parsing errors in redb::Value? #802

Closed flokli closed 5 months ago

flokli commented 5 months ago

I'd like to use redb to store structs and use them as value in a TableDefinition, so I assumed the right way to do this is implementing redb::Value for my specific type.

My structs already have a serialization/deserialization to/from bytes - they're generated from protobuf definitions using prost, and I can just call .encode_to_vec() or MyStruct::decode(data).

There's one big catch though - decoding can fail (assuming disk corruption, an old version writing wrong data by mistake, …) - and the trait currently doesn't allow me to pass along such a decoding error.

How is this supposed to be handled? Or am I holding the whole thing wrong, and I shouldn't implement redb::Value for my own types at all?

cberner commented 5 months ago

You would panic in that case.

If disk corruption has occurred the whole database is likely irrecoverable anyway.

And for old versions of code, you probably want a separate version check. Like another table in which you store the schema version.

If you really need fallible decoding, then the way to do it is to store a &[u8] and do the decoding outside redb

flokli commented 5 months ago

Mmh, I still think panick'ing is a bit too drastic - let's say I store the data in two places, and want to fall back to reading it in the other place if it's corrupted/wrong, without panicking.

I'd prefer if that trait have be fallible method. FWIW, the redb Result type itself also has a Corrupted or Io enum kind, and doesn't just panic in that case.

flokli commented 5 months ago

In any case, could this documented a bit more prominently in the trait docstring? I'm probably not the only one wondering about it.

cberner commented 5 months ago

I decided that it was better to not return a Result, because if it returns Result then AccessGuard::value would have to return a Result too. If you need to return a Result, the other thing you can do is implement Value for Result instead of YourType

Sent from my phone

On Fri, May 3, 2024, 9:44 AM Florian Klink @.***> wrote:

Mmh, I still think panick'ing is a bit too drastic - let's say I store the data in two places, and want to fall back to reading it in the other place if it's corrupted/wrong, without panicking.

I'd prefer if that trait have be fallible method. FWIW, the redb Result type itself also has a Corrupted or Io enum kind, and doesn't just panic in that case.

— Reply to this email directly, view it on GitHub https://github.com/cberner/redb/issues/802#issuecomment-2093380732, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGNXQFKZ7YVBKIUDJUBMETZAO5HLAVCNFSM6AAAAABHFPUWW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJTGM4DANZTGI . You are receiving this because you commented.Message ID: @.***>

flokli commented 5 months ago

Hmmh, the API would look quite different - as the user now would need to explicitly provide the Ok(_) value, and the trait impl would need to handle having an Err case passed. I think I'd rather prefer storing Vec<u8> in the database and doing the parsing as a followup step instead.