Parachains db column "migration"

[sandreim]

This is a followup of #5594 . This aims to be a "catch" all migration for any column option changes such that no manual fixing for ParityDB is required. Should also work if client is rolled back.

[sandreim]

Tested this locally, but a Versi burn in is needed to see how much time migration takes and if there are any issues afterwards

[vstakhov]

I was about to suggest the same 👍 It is wa-ay better to provide a clear migration path than to crash on upgrade with no cleaning up.

[vstakhov]

The only issue was that paritydb has stored unordered columns as hashes only with no values, so @eskimor told that there is no way to migrate with data preservation. Have you tried that such a migration does really work?

[sandreim]

The only issue was that paritydb has stored unordered columns as hashes only with no values, so @eskimor told that there is no way to migrate with data preservation. Have you tried that such a migration does really work?

Not sure what happens to the data right now, it shouldn’t be preserved. Its a soft wipeout and hopefully there is no dangling stuff. The good partt is that there is no startup error now and no need to manually ‘fix’ the db.

[arkpar]

I don't think migrate works correctly for moving data from hash column to btree.
Unless the previous version set the uniform flag for the colum, there's no way to recover the original key, as the database does not store it.
@cheme Please correct me if I'm wrong here.

Why not just delete the parachains database instead? As far as I know it should be re-populated in a matter of a few blocks.

[cheme]

Can only confirm, only 'uniform' column can be migrated.

[cheme]

Migrate function here should work when changing configuration of hash indexed column (hash indexed to hash indexed), and also with the same parameters (can gain reclaim some space sometime).

When migrating from hash indexed to btree, what may currently happen is that it migrate the index and values tables (the btree_indexed option parameter is currently ignored by the migration). After that, probably opening the db with btree_index act as if the column was deleted (no entry may have been written where the btree root usually is) and things may run fine.

For btree indexed column the migrate function is unimplemented, but should be easy to implement (just run the iterator on the whole db and inject all key values to the target column) and would allow to switch from btree indexed to hash indexed.

The fact that we cannot recover key of the content stored in column may be a concern in some case, idea should be to attach key to value then or implement a variant that keep original key along the value (or in an attached value entry). But that is a different question.

Also depending on context if a single column needs to be removed, deleting the metadata file and all index_X_* and table_X_* with X the column number should work (almost untested).

[vstakhov]

Why not just delete the parachains database instead?

It would be enough to delete dispute coordinator column I suppose. The goal here is to provide more or less smooth migration. Even a shell script that performs rm -fr is better than crashing on an incorrect database.

[sandreim]

I was hoping for a more sane solution especially on the user experience. Automated db removal sounds a bit scary to me.

[cheme]

+1 on the fact that automating a column removal is scary, maybe if we know some column are allowed to be removed we can hardcode the info and only run this on such column.
But more generally, I think a warning should be issued before (maybe even a confirmation): synching the data can take time and users should know the node may be a bit non responsive.

[sandreim]

+1 on the fact that automating a column removal is scary, maybe if we know some column are allowed to be removed we can hardcode the info and only run this on such column. But more generally, I think a warning should be issued before (maybe even a confirmation): synching the data can take time and users should know the node may be a bit non responsive.

I've implemented it to work only for 1 column (dispute coordinator data) and will spit out warnings. There shouldn't be any issues with sync and responsiveness AFAIK.

[arkpar]

Database implementation details w.r.t. file names may change in the future and this will break. There should be a function for clearing a column provided by the database. I've added one here:
paritytech/parity-db#84

[sandreim]

Thanks, you are right, this relies on impl details. I’d expand on that - move most of this code in paritydb and add an additional column option to delete it when no migration is available. Would this work?

[arkpar]

Not sure this is a good idea. Mixing actual configuration options with some flags that specify behaviour for some edge cases. I'd rather have it simple: try to open the database in the user code. If there's a particular error, the user code may handle it by dropping the column. I've added an explicit error type IncompatibleColumnConfig to make it possible to detect it better.

[ordian]

IMHO we should try to clear a column only if opening the db fails with some specific error. Other than that, lgtm.

[sandreim]

IMHO we should try to clear a column only if opening the db fails with some specific error. Other than that, lgtm.

I think this is currently more efficient and gives more control on how the issue is resolved, because we can detect the condition ourselves, and doesn't require to have a loop in which to open and delete on error. If you have a strong opinion about this I would change it.

[ordian]

IMHO we should try to clear a column only if opening the db fails with some specific error. Other than that, lgtm.

I think this is currently more efficient and gives more control on how the issue is resolved, because we can detect the condition ourselves, and doesn't require to have a loop in which to open and delete on error. If you have a strong opinion about this I would change it.

It doesn't need to be a loop, just opening at most 2 times (regular open, and in case of this specific error, try clearing a column and open again (w/o a retry), otherwise, propagate the error). My main worry is that generally, we don't want to wipe the parachains db (or part of it) on all nodes, esp once paritydb replaces rocksdb and we do this on more than 1/3 of validators. This could lead to security and/or consensus issues.

[sandreim]

IMHO we should try to clear a column only if opening the db fails with some specific error. Other than that, lgtm.

I think this is currently more efficient and gives more control on how the issue is resolved, because we can detect the condition ourselves, and doesn't require to have a loop in which to open and delete on error. If you have a strong opinion about this I would change it.

It doesn't need to be a loop, just opening at most 2 times (regular open, and in case of this specific error, try clearing a column and open again (w/o a retry), otherwise, propagate the error). My main worry is that generally, we don't want to wipe the parachains db (or part of it) on all nodes, esp once paritydb replaces rocksdb and we do this on more than 1/3 of validators. This could lead to security and/or consensus issues.

Assuming one column needs clearing, yeah 2 db opens would work, but if we don;t really know the number of cols which need fixing we need to loop and err on each col index and then fix it. If we use the version file bumper, then we would need manually hardcode the migration explicitly like it works for rocks db.

[sandreim]

I've implemented the versioning for ParityDB which was missing and added the column clear code as a parityDB migration from anything in the past to current version.

[sandreim]

@bkchr @ordian PTAL

[ordian]

We should test this, but looks reasonable.

[sandreim]

bot merge