Database versioning and migration #3255
Conversation
sudo-shashank
requested review from
lemmih and
LesnyRumcajs
and removed request for
a team
There was a problem hiding this comment.
You struggle with setting up a framework for database migrations because we have yet to have any database migrations, imo.
Try setting up a database migration from version Unknown to version 0.12.0-rc (unreleased version, you can pretend it exists) to version dev. Migrating between three different versions should give you a lot of experience.
| PathBuf::from(&config.client.data_dir).join(config.chain.network.to_string()) | ||
| let chain_path = PathBuf::from(&config.client.data_dir).join(config.chain.network.to_string()); | ||
| // Use the dev database if it exists, else use versioned database | ||
| let dev_path = chain_path.join("dev"); |
There was a problem hiding this comment.
When is the database ever put in the dev folder?
src/db/migration/mod.rs
Outdated
| /// Database version for each forest version which supports db migration | ||
| #[derive(Debug, Eq, PartialEq)] | ||
| pub enum DBVersion { | ||
| V0, // Default DBVersion for any unknown db |
There was a problem hiding this comment.
If the version is unknown, let's call it Unknown rather than V0.
src/db/migration/mod.rs
Outdated
| use std::sync::Arc; | ||
| use tracing::info; | ||
|
|
||
| pub const LATEST_DB_VERSION: DBVersion = DBVersion::V11; |
There was a problem hiding this comment.
Why manually define this? It could be inferred from the version number of Forest.
src/db/migration/mod.rs
Outdated
| #[derive(Debug, Eq, PartialEq)] | ||
| pub enum DBVersion { | ||
| V0, // Default DBVersion for any unknown db | ||
| V11, |
There was a problem hiding this comment.
Why is this version 11? Forest is at version 0.11.1. How about you just use https://docs.rs/semver/latest/semver/struct.Version.html to exactly capture the Forest version?
| db_root(existing_chain_data_root), | ||
| config.db_config().clone(), | ||
| )?); | ||
| let genesis = read_genesis_header(None, config.chain.genesis_bytes(), &db).await?; |
There was a problem hiding this comment.
You probably want ts.genesis().
src/db/migration/mod.rs
Outdated
|
|
||
| let ts = state_manager.chain_store().heaviest_tipset(); | ||
| let height = ts.epoch(); | ||
| assert!(height.is_positive()); |
There was a problem hiding this comment.
I don't understand the purpose of this assertion. More documentation is needed here.
There was a problem hiding this comment.
not required, removed
src/db/migration/mod.rs
Outdated
| let next_version = match current_version { | ||
| DBVersion::V0 => DBVersion::V11, | ||
| _ => break, | ||
| }; |
There was a problem hiding this comment.
I need help understanding the details of this code. The list of Forest versions is static, and we shouldn't have to specify the next_version manually. For example, the last three Forest versions are: ["0.10.0", "0.11.0", "0.11.1"]. Keeping the versions in a vector tells you about the known versions and their ordering.
There was a problem hiding this comment.
I am maintaining the list of versions now
src/db/migration/mod.rs
Outdated
|
|
||
| // TODO: Add Steps required for new migration | ||
| /// Migrate to an intermediate db version | ||
| fn migrate(_existing_db_path: &Path, next_version: &DBVersion) -> anyhow::Result<()> { |
There was a problem hiding this comment.
It looks like you want the migrations to happen inside the database folder. That's not safe. If something goes wrong, we want to revert back to the last known good state.
There was a problem hiding this comment.
I am currently using temp_dir for intermediate db migrations, if something goes wrong we can revert back to last good state easily
There was a problem hiding this comment.
Database versioning and migration are crucial to having Forest working and resolving issues on running services. This is a feature that needs fancy diagrams and team wide-understanding.
How about you make a proposal (not in the form of code) and present it to the team after daily?
sudo-shashank
force-pushed
the
shashank/db-migration
branch
from
ead091a
to
18a6140
Compare
Summary of changes
Changes introduced in this pull request:
devdatabase if it existReference issue to close (if applicable)
Closes #3203
Other information and links
Change checklist