Store the database in a role specific subdirectory #8658

hirschenberger · 2021-04-22T10:51:36Z

If there is already a database in the basedir, it will be migrated
(moved) into the role-subdirectory.

polkadot companion: paritytech/polkadot#2923

TODO:

Create an error when trying to migrate the directory if the database-role and the role of the running client differs.

If there is already a database in the basedir, it will be migrated (moved) into the role-subdirectory. fixes paritytech#6880

… issue_6880

…t role fails

hirschenberger · 2021-04-23T14:11:28Z

There's one issue when the client is started in a different role then the database (before migration to the subdirectory), the db gets moved to the subdirectory of the client, not the db. And opening the database later will fail.
Unfortunately then the db is already moved and will stay in the wrong directory, making subsequent starts also fail.

Unfortunately there's no stable way of finding out the DatabaseType in the client before it is opened. But the migration of the database to the subdir must be in this early phase, as the later code must not depend or know any special paths or directory layouts.

I tried to implement a database_type(path) -> Result<DatabaseType> function in the Backend which opens the databases in all modes and checks for failure. Unfortunately I can't call that function from the client, because I don't know the Block type of the backend in this phase.

…ype before migration

hirschenberger · 2021-04-24T10:37:11Z

Ready for review.

We better rely on directory names for migration-detection

… issue_6880

stale · 2021-07-07T16:12:53Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

hirschenberger · 2021-07-07T17:01:26Z

This issue just needs a review.

arkpar · 2021-08-04T16:37:51Z

bin/node/cli/tests/common.rs

@@ -65,3 +65,39 @@ pub fn run_dev_node_for_a_while(base_path: &Path) {
 	kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap();
 	assert!(wait_for(&mut cmd, 40).map(|x| x.success()).unwrap_or_default());
 }
+
+/// Run the node for a while (30 seconds)
+pub fn run_node_with_args_for_a_while(base_path: &Path, args: &[&str]) {


run_dev_node_for_a_while could be implemented in terms of run_node_with_args_for_a_while now

eg https://github.com/cheme/substrate/blob/844fb80e0195ed05a2ecc1d95baa0c5760278388/bin/node/cli/tests/common.rs#L50

Or the caller can just pass &[--dev] as args to run_node_with_args_for_a_while

arkpar · 2021-08-04T16:46:59Z

client/cli/src/config.rs

-		})
+		};
+
+		if let Some(p) = db_conf.path() {


It looks like the following migration code belongs in sc-client or sc-client-db. Right before opening the database. Cli should not be concerned with database migration. It would also remove the dependency on sc-client-db.

Sure, this was the first thing I tried, but I actually struggled at this problem a bit, because the migration code needs to know the Role in which the client was started and the DatabaseType to be sure not to move the db to the wrong directory.

When I tried to inject the Role from the Cli to the client I had a hard time with the compiler. That's why I chose it the other way around. But maybe moving the Role enum to sc-client would work.

Role is passed down as db_type here

substrate/client/db/src/utils.rs

Line 208 in 1d5abf0

pub fn open_database<Block: BlockT>(

Not really, there's two distinct (and overlapping) terms.

There's Role from https://github.com/paritytech/substrate/blob/master/client/network/src/config.rs#L148
and DatabaseType from https://github.com/paritytech/substrate/blob/master/client/db/src/utils.rs#L91

Role is coming from the cli as parameter and DatabaseType is determined from the database.

As far as understand, db_type that's passed to open_database is derived from the CLI argument. The CLI argument selects type of backend to use. Light backend opens the database as light here: https://github.com/paritytech/substrate/blob/master/client/db/src/light.rs#L76
And full backend opens it as full here: https://github.com/paritytech/substrate/blob/master/client/db/src/lib.rs#L1082

open_database does indeed check that expected db_type matches the one in the database. But you can still use expected db_type in the same way you use role w.r.t. migration.

But the check for the correct database happens after the migration was applied and the database was opened, so on a mismatch I have to roll back the directories and error?

If you check the database type before migration there should be no mismatch.

I'm suggesting moving your migration code inside the open_database function.
Basically move all of the code from open_database to a helper function, e.g. open_database_at
So that the new version of open_database would do something like this:

Check the base path for an existing database. Try to open it with open_database_at, which will validate expected db_type. If it succeeds, close the database and do the migration.

Append "full" or "light" to base path. Open it with open_database_at and return

A bit of refactoring might be required to extract the meat of open_database to a helper function that does the heavy lifting of actually opening the database.

arkpar · 2021-08-04T16:54:48Z

client/db/src/lib.rs

+        }
+
+		if crate::utils::open_database::<Block>(&config, DatabaseType::Light).is_ok() {
+			return Ok(DatabaseType::Light);


This method of detecting database type is a bit hacky. It relies on the fact that the databases have different number of columns. Also unlike rocksdb, paritydb does not care if you don't open all of the columns. So it is important to try for full database first, before trying as light. Could you please add a comment to that extend?

Skimming through code I just read that open_database also call

substrate/client/db/src/utils.rs

Line 290 in b7409a2

pub fn check_database_type(

so should be fine?

Right, there's already a method for it

This was the best way I could find to differentiate them, will add a comment for full disclosure.

If you move migration code to sc-client-db you can just use utils::check_database_type instead.

On the other hand utils::check_database_type requires the database to be open already, which in turn requires specifying a DatabaseType. So I guess the current approach is fine after all.

I remember, that was the problem.

arkpar · 2021-08-04T16:56:51Z

Thanks for the PR an apologies for the late review. Totally agree that the databases should be separated.
Looks good, apart from migration code should be moved to a different crate.

hirschenberger · 2021-08-12T17:29:11Z

There had been some refactorings in master since this PR was created, so I had to rework some stuff. I'm nearly finished but it still needs some tweaks on the tests.

I'm in holiday for three weeks now but I'll create a new PR based on master when I'm back. Cu...

This is a cleaned up version of paritytech#8658 fixing paritytech#6880 polkadot companion: paritytech/polkadot#2923

hirschenberger · 2021-08-28T18:32:53Z

Back from honeymoon, I created a new PR #9645 based on current master.

* Store the database in a role specific subdirectory This is a cleaned up version of #8658 fixing #6880 polkadot companion: paritytech/polkadot#2923 * Disable prometheus in tests * Also change p2p port * Fix migration logic * Use different identification file for rocks and parity db Add tests for paritydb migration

hirschenberger added 3 commits April 22, 2021 12:48

Store the database in a role specific subdirectory

f4c53af

If there is already a database in the basedir, it will be migrated (moved) into the role-subdirectory. fixes paritytech#6880

Merge remote-tracking branch 'origin/master' into issue_6880

b284911

Merge branch 'master' of https://github.com/paritytech/substrate into…

296f080

… issue_6880

ordian added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Apr 22, 2021

Add test if migrating a database with a different role then the clien…

b725354

…t role fails

hirschenberger added 2 commits April 24, 2021 12:19

Add checks if the node was started in the same role as the database t…

3db92c4

…ype before migration

Typo and trigger CI

ee9c971

hirschenberger added 2 commits April 25, 2021 10:55

The file db_version is not always present

84c0ef0

We better rely on directory names for migration-detection

Merge branch 'master' of https://github.com/paritytech/substrate into…

1c8e78f

… issue_6880

paritytech deleted a comment from cla-bot-2021 bot Jun 3, 2021

hirschenberger mentioned this pull request Jun 24, 2021

ParityDb stabilization tracking #9201

Closed

5 tasks

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021

hirschenberger mentioned this pull request Jul 24, 2021

Light client errors: node-template --light #9352

Closed

arkpar reviewed Aug 4, 2021

View reviewed changes

arkpar requested review from bkchr, cheme and debris August 4, 2021 16:57

hirschenberger added a commit to hirschenberger/substrate that referenced this pull request Aug 28, 2021

Store the database in a role specific subdirectory

31be7a7

This is a cleaned up version of paritytech#8658 fixing paritytech#6880 polkadot companion: paritytech/polkadot#2923

hirschenberger mentioned this pull request Aug 28, 2021

Store the database in a role specific subdirectory #9645

Merged

hirschenberger closed this Aug 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store the database in a role specific subdirectory #8658

Store the database in a role specific subdirectory #8658

hirschenberger commented Apr 22, 2021 •

edited

Loading

hirschenberger commented Apr 23, 2021

hirschenberger commented Apr 24, 2021

stale bot commented Jul 7, 2021

hirschenberger commented Jul 7, 2021

arkpar Aug 4, 2021

cheme Aug 4, 2021

hirschenberger Aug 4, 2021

arkpar Aug 4, 2021

hirschenberger Aug 4, 2021

arkpar Aug 4, 2021

hirschenberger Aug 4, 2021

arkpar Aug 4, 2021 •

edited

Loading

hirschenberger Aug 4, 2021

arkpar Aug 4, 2021

arkpar Aug 4, 2021

cheme Aug 4, 2021

arkpar Aug 4, 2021

hirschenberger Aug 4, 2021

arkpar Aug 4, 2021

arkpar Aug 4, 2021

hirschenberger Aug 4, 2021

arkpar commented Aug 4, 2021 •

edited

Loading

hirschenberger commented Aug 12, 2021

hirschenberger commented Aug 28, 2021

Store the database in a role specific subdirectory #8658

Store the database in a role specific subdirectory #8658

Conversation

hirschenberger commented Apr 22, 2021 • edited Loading

TODO:

hirschenberger commented Apr 23, 2021

hirschenberger commented Apr 24, 2021

stale bot commented Jul 7, 2021

hirschenberger commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Aug 4, 2021 • edited Loading

hirschenberger commented Aug 12, 2021

hirschenberger commented Aug 28, 2021

hirschenberger commented Apr 22, 2021 •

edited

Loading

arkpar Aug 4, 2021 •

edited

Loading

arkpar commented Aug 4, 2021 •

edited

Loading