Force rollback of tables with foreign keys / cascading deletes on automatic db migrations #1290

KtorZ · 2020-01-20T17:53:10Z

Issue Number

Overview

74c232e
force rollback to genesis after automated migrations
persistent clear foreign tables on automatic migration due to
cascading delete. One possible approach to solve this: forcing a
manual rollback to genesis whenever there are migrations applied
to the database, regardless of the migration. Only the foreign
tables are affected, so static metadata and wallet credentials
are preserved. This mean that we would force resync of wallets
on every updates that have database changes, but that's a fair
price to pay for now.
de3825d
remove foreign keys to 'Wallet' table and cascade deletes manually
This to prevent 'persistent' from doing it during automated migration! We don't do
this for checkpoints because the cascading updates are truly helpful there to manage
the database state and, can be more easily rolled back at any time (since the data
regarding checkpoints entirely comes from the chain).

Comments

~~⚠️ Currently manually testing db migration from various previous versions.~~

Automated migration passes! And on old version, we have a nice log showing what's going on, including manual migration, and handling of automated ones!

 *** Migrating from v2019-12-13 ***

[cardano-wallet.application:Info:22] [2020-01-20 17:58:12.47 UTC] Found existing wallet: 99ef09bdab5f62579f2d6db02ba7df5e88b7e865
[cardano-wallet.wallet-db:Notice:28] [2020-01-20 17:58:12.47 UTC] checkpoint table does not contain required field 'active_slot_coeff'. Adding this field with a default value of 1.0.
[cardano-wallet.wallet-db:Notice:28] [2020-01-20 17:58:12.52 UTC] 48 migrations were applied to the database.
[cardano-wallet.wallet-db:Notice:28] [2020-01-20 17:58:12.52 UTC] Some automated database migrations happened. Forced to rollback to genesis.
[cardano-wallet.main:Info:22] [2020-01-20 17:58:12.53 UTC] Wallet backend server listening on 127.0.0.1:8090
[cardano-wallet.wallet-engine:Info:28] [2020-01-20 17:58:12.53 UTC] 99ef09bd: Applying blocks [1167051.7 ... 1167051.7]
[cardano-wallet.wallet-engine:Info:28] [2020-01-20 17:58:12.53 UTC] 99ef09bd: Creating checkpoint at 3b2894d8-[1167051.7#1]

So, to summarize what's happening:

An existing wallet database is found.
Before running any automatic migration, the database is inspected and an extra column is found. So we manually alter the table to add a column with a default value.
Then, persistent runs its own set of automated migrations on top of the database, running 48 of them.
This is then detected and induces immediately a rollback to genesis of all the on-chain data.
The wallet starts applying blocks and creating checkpoints, starting with a checkpoint at height #1 (right after genesis)

🎉

KtorZ · 2020-01-20T18:05:31Z

bors try

KtorZ · 2020-01-20T18:29:45Z

bors r-

iohk-bors · 2020-01-20T18:29:59Z

try

Build failed

buildkite/cardano-wallet

KtorZ · 2020-01-20T18:30:00Z

bors try

iohk-bors · 2020-01-20T19:02:01Z

try

Build failed

buildkite/cardano-wallet

jonathanknowles

Hi @KtorZ

I've had a chance to test your PR, and I was able to successfully start with a wallet that was created with the v2019-12-13 branch. So far so good! 🍾

I do have a couple of questions / concerns though:

The trigger condition for performing a forceful rollback is that we see a nonzero migration count. However, this count currently only includes automatic migrations. Should we not also include manual migrations in this count? It's conceivable that a successful manual migration will bring the database into a state where no automatic migrations are necessary. But it might be wiser to err on the side of caution and still perform a forceful rollback in this case. (We can always remove this in future, if we're more confident, and have more testing.)
The removal of automatic cascading deletes. The nice thing about having cascading deletes is that we can rely on the database implementation to ensure referential integrity (no dangling references). Even in the case where Persistent's automatic migration mechanism (erroneously) causes rows to be deleted in tables that reference the checkpoint table, we can still rely on SQLite to preserve referential integrity. However, if we remove this automatic mechanism for ensuring referential integrity, do we not increase our testing burden? Haven't we now made it easier for future code to break referential integrity?

Apart from these concerns, this PR looks good to me. I have a couple of comments regarding Haddock comments and names of types.

jonathanknowles · 2020-01-21T02:47:40Z

lib/core/src/Cardano/DB/Sqlite.hs

@@ -211,7 +213,7 @@ startSqliteBackend
    -> Migration
    -> Tracer IO DBLog
    -> Maybe FilePath
-    -> IO (Either MigrationError SqliteContext)
+    -> IO (Either MigrationError (Quantity "migration" Int, SqliteContext))


Suggestion: use migrations (plural) or migrationCount here, to really emphasize that it is a count of the number of migrations applied.

I disagree here ^.^
The Symbol attached with a Quantity is used as a unit. We have several occurrences of this in the source code, like Quantity "lovelace" ... or Quantity "block" ..., always singular. Same applies here IMO.

yeah, Quantity "migration" is complied with convention adopted in other places

jonathanknowles · 2020-01-21T02:55:07Z

lib/core/src/Cardano/DB/Sqlite.hs

@@ -231,7 +233,8 @@ startSqliteBackend manualMigration migrateAll trace fp = do
        Left e -> do
            destroyDBLayer ctx
            pure $ Left e
-        Right _ -> pure $ Right ctx
+        Right ms -> do
+            pure $ Right (Quantity (length ms), ctx)


Question: this count (ms) appears to only include automatic migrations.

Should we not also include any manual migrations in this total count?

So: manual migrations + automatic migrations?

I'm assuming it's possible for there to be cases where there are manual migrations but no automatic migrations. (This would happen if our manual migrations successfully produce the exact schema that Sqlite is expecting.)

Your assumption is correct: there can be manual migrations but no automatic ones.
Still, the migration count here (and this particular fix) is solely about automatic ones. It comes as a work-around for persistent's behavior of deleting tables combined with the cascading deletion of foreign tables.
For manual migration, we are in total control of what's being done and, it is very unlikely we would make a manual migration that requires rolling back the chain. If we ever do, then it'd be probably time to revisit this whole migration logic instead of patching up things on top of each other.
The migration logic has become quite convoluted already. Manual migrations shouldn't be included because they aren't causing implicit uncontrolled behavior here.

@KtorZ wrote:

For manual migration, we are in total control of what's being done and, it is very unlikely we would make a manual migration that requires rolling back the chain. If we ever do, then it'd be probably time to revisit this whole migration logic instead of patching up things on top of each other.

Fair enough! This is a convincing reason.

lib/core/src/Cardano/DB/Sqlite.hs

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

`persistent` clear foreign tables on automatic migration due to cascading delete. One possible approach to solve this: forcing a manual rollback to genesis whenever there are migrations applied to the database, regardless of the migration. Only the foreign tables are affected, so static metadata and wallet credentials are preserved. This mean that we would force resync of wallets on every updates that have database changes, but that's a fair price to pay for now.

This to prevent 'persistent' from doing it during automated migration! We don't do this for checkpoints because the cascading updates are truly helpful there to manage the database state and, can be more easily rolled back at any time (since the data regarding checkpoints entirely comes from the chain).

paweljakubas · 2020-01-21T11:41:24Z

lib/core/src/Cardano/DB/Sqlite.hs

+            [ "Some automated database migrations happened. Forced to roll back "
+            , "to genesis."


minor suggestion

Some automated database migrations happened. Rollback to the genesis was forced.

it is somewhat more natural to me

Why the however 🤔 ?

yeah, probably without the. so no to the genesis but to genesis

paweljakubas · 2020-01-21T11:42:58Z

lib/core/src/Cardano/Pool/DB/Sqlite.hs

+    let start = startSqliteBackend (ManualMigration mempty) migrateAll trace fp
+    (nMigrations, ctx) <- handlingPersistError trace fp start
+    let dbLayer = mkDBLayer ctx
+    when (nMigrations > Quantity 0) $ do


paweljakubas · 2020-01-21T11:45:42Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+-- tables, and re-creating them from scratch. When paired with automatic cascade
+-- deletion, this has dramatic consequences on the wallet data integrity (when a
+-- source table is deleted due to a migration, all foreign tables are also
+-- deleted, but only the rows in the source table are replaced!).


💯 for the comment above

paweljakubas · 2020-01-21T11:47:17Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

@@ -424,7 +424,15 @@ newDBLayer trace defaultFieldValues mDatabaseFile = do
            selectWallet wid >>= \case
                Nothing -> pure $ Left $ ErrNoSuchWallet wid
                Just _  -> Right <$> do
-                    deleteCascadeWhere [WalId ==. wid]
+                    deleteWhere [WalId ==. wid]


if so then ON DELETE CASCADE in TH.hs are to be deleted

and they are

KtorZ · 2020-01-21T11:52:18Z

bors r+

1290: Force rollback of tables with foreign keys / cascading deletes on automatic db migrations r=KtorZ a=KtorZ # Issue Number  #1279 # Overview  - 74c232e force rollback to genesis after automated migrations `persistent` clear foreign tables on automatic migration due to cascading delete. One possible approach to solve this: forcing a manual rollback to genesis whenever there are migrations applied to the database, regardless of the migration. Only the foreign tables are affected, so static metadata and wallet credentials are preserved. This mean that we would force resync of wallets on every updates that have database changes, but that's a fair price to pay for now. - de3825d remove foreign keys to 'Wallet' table and cascade deletes manually This to prevent 'persistent' from doing it during automated migration! We don't do this for checkpoints because the cascading updates are truly helpful there to manage the database state and, can be more easily rolled back at any time (since the data regarding checkpoints entirely comes from the chain). # Comments  ~~:warning: Currently manually testing db migration from various previous versions.~~ Automated migration passes! And on old version, we have a nice log showing what's going on, including manual migration, and handling of automated ones! ``` *** Migrating from v2019-12-13 *** [cardano-wallet.application:Info:22] [2020-01-20 17:58:12.47 UTC] Found existing wallet: 99ef09bdab5f62579f2d6db02ba7df5e88b7e865 [cardano-wallet.wallet-db:Notice:28] [2020-01-20 17:58:12.47 UTC] checkpoint table does not contain required field 'active_slot_coeff'. Adding this field with a default value of 1.0. [cardano-wallet.wallet-db:Notice:28] [2020-01-20 17:58:12.52 UTC] 48 migrations were applied to the database. [cardano-wallet.wallet-db:Notice:28] [2020-01-20 17:58:12.52 UTC] Some automated database migrations happened. Forced to rollback to genesis. [cardano-wallet.main:Info:22] [2020-01-20 17:58:12.53 UTC] Wallet backend server listening on 127.0.0.1:8090 [cardano-wallet.wallet-engine:Info:28] [2020-01-20 17:58:12.53 UTC] 99ef09bd: Applying blocks [1167051.7 ... 1167051.7] [cardano-wallet.wallet-engine:Info:28] [2020-01-20 17:58:12.53 UTC] 99ef09bd: Creating checkpoint at 3b2894d8-[1167051.7#1] ``` So, to summarize what's happening: 1. An existing wallet database is found. 2. Before running any automatic migration, the database is inspected and an extra column is found. So we manually alter the table to add a column with a default value. 3. Then, persistent runs its own set of automated migrations on top of the database, running 48 of them. 4. This is then detected and induces immediately a rollback to genesis of all the on-chain data. 5. The wallet starts applying blocks and creating checkpoints, starting with a checkpoint at height `#1` (right after genesis) :tada:  Co-authored-by: KtorZ <matthias.benkort@gmail.com>

iohk-bors · 2020-01-21T12:25:06Z

Build succeeded

buildkite/cardano-wallet

KtorZ requested a review from paweljakubas January 20, 2020 17:53

KtorZ self-assigned this Jan 20, 2020

iohk-bors bot added a commit that referenced this pull request Jan 20, 2020

Try #1290:

a91dc98

KtorZ force-pushed the KtorZ/force-rollback-automated-migrations branch from 7a18339 to de3825d Compare January 20, 2020 18:20

KtorZ force-pushed the KtorZ/force-rollback-automated-migrations branch from de3825d to ce7db9a Compare January 20, 2020 18:29

iohk-bors bot added a commit that referenced this pull request Jan 20, 2020

Try #1290:

481055f

jonathanknowles reviewed Jan 21, 2020

View reviewed changes

KtorZ added 2 commits January 21, 2020 10:37

KtorZ force-pushed the KtorZ/force-rollback-automated-migrations branch from 6b70b75 to 0583144 Compare January 21, 2020 09:37

paweljakubas reviewed Jan 21, 2020

View reviewed changes

paweljakubas approved these changes Jan 21, 2020

View reviewed changes

iohk-bors bot merged commit 0583144 into master Jan 21, 2020

KtorZ deleted the KtorZ/force-rollback-automated-migrations branch January 21, 2020 14:13

KtorZ mentioned this pull request Jan 21, 2020

persistent clear foreign tables on automatic migration due to cascading delete #1279

Closed

jonathanknowles mentioned this pull request Feb 10, 2020

Suppress automatic migrations for pre-existing wallet databases. #1334

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force rollback of tables with foreign keys / cascading deletes on automatic db migrations #1290

Force rollback of tables with foreign keys / cascading deletes on automatic db migrations #1290

KtorZ commented Jan 20, 2020 •

edited

Loading

KtorZ commented Jan 20, 2020

KtorZ commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

KtorZ commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

jonathanknowles left a comment

jonathanknowles Jan 21, 2020

KtorZ Jan 21, 2020

paweljakubas Jan 21, 2020

jonathanknowles Jan 21, 2020 •

edited

Loading

KtorZ Jan 21, 2020

jonathanknowles Jan 21, 2020

paweljakubas Jan 21, 2020 •

edited

Loading

KtorZ Jan 21, 2020

paweljakubas Jan 21, 2020

paweljakubas Jan 21, 2020

paweljakubas Jan 21, 2020 •

edited

Loading

paweljakubas Jan 21, 2020 •

edited

Loading

paweljakubas Jan 21, 2020

KtorZ commented Jan 21, 2020

iohk-bors bot commented Jan 21, 2020

		[ "Some automated database migrations happened. Forced to roll back "
		, "to genesis."

Force rollback of tables with foreign keys / cascading deletes on automatic db migrations #1290

Force rollback of tables with foreign keys / cascading deletes on automatic db migrations #1290

Conversation

KtorZ commented Jan 20, 2020 • edited Loading

Issue Number

Overview

Comments

KtorZ commented Jan 20, 2020

KtorZ commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

try

Build failed

KtorZ commented Jan 20, 2020

iohk-bors bot commented Jan 20, 2020

try

Build failed

jonathanknowles left a 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

jonathanknowles Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Jan 21, 2020 • 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

paweljakubas Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

paweljakubas Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Jan 21, 2020

iohk-bors bot commented Jan 21, 2020

Build succeeded

KtorZ commented Jan 20, 2020 •

edited

Loading

jonathanknowles Jan 21, 2020 •

edited

Loading

paweljakubas Jan 21, 2020 •

edited

Loading

paweljakubas Jan 21, 2020 •

edited

Loading

paweljakubas Jan 21, 2020 •

edited

Loading