search

mozilla / rkv

A simple, humane, typed key-value storage solution.
https://crates.io/crates/rkv
Apache License 2.0
304 stars 52 forks source link

Migration updates #206

Closed victorporof closed 3 years ago

victorporof commented 3 years ago

This PR does 2 things:

  1. Fixes bug https://bugzilla.mozilla.org/show_bug.cgi?id=1663856

This is a fun one. If old migrated data isn't deleted (e.g. when there's multiple environments open at the same path and deleting the files would be invalid while another environment is still open), it's possible that databases with new a different storage drivers (e.g. safe-mode) continue to be created and end up populated at runtime. Next time migration is attempted, migration doesn't fail silently anymore (since the destination is not empty). See comment 5).

I've added some tests for this and now handling the DestinationNotEmpty situations. In every situation, the consumer code that's using this helper:

  1. Prevent potential deadlocks in safe mode environments when opening dbs.

Two rwlocks is one too many. This is theoretical and nobody's encountered this yet afaik, but is caught by tsan: https://bugzilla.mozilla.org/show_bug.cgi?id=1606804. Switching to a single rwlock should fix it I think, and it shouldn't hurt in general.

victorporof commented 3 years ago

You're correct, migration would be attempted again next time, but skipped because the destination is not empty. I definitely agree that merging would be quite elegant. I'd be very happy to get a PR for this, but unlikely to implement this for now given our particular use cases where we don't care that much about the old data (and merging is a tricky problem in general!).

Thanks for the quick review.