Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recover from partial cleanupCheckpointTable db migration #2503

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Feb 10, 2021

Issue Number

ADP-700

Overview

  • Test case to reproduce the migration error "Database migration: manual intervention required." (needed manual fiddling of the database - we could not reproduce it otherwise)
  • Adjust the field checks in the checkpoint cleanup migration, so that it can recover from the situation (most of the time).

@rvl rvl self-assigned this Feb 10, 2021
@rvl rvl requested review from KtorZ and Anviking February 10, 2021 13:06
[defaults] <- runSql conn $ select ["genesis_hash", "genesis_start"] orig
let [PersistText genesisHash, PersistText genesisStart] = defaults
addColumn_ conn True (DBField WalGenesisHash) (quotes genesisHash)
addColumn_ conn True (DBField WalGenesisStart) (quotes genesisStart)

-- 2. Drop columns from the 'checkpoint' table
-- 2. Drop columns from the 'checkpoint' table
isFieldPresentByName conn "checkpoint" "genesis_hash" >>= \case
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the checkpoint table contains the genesis_hash, we do the below migration...? Won't this always be the case? Particularly after the step above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got confused by that at first too. Yet, it should work fine because we are actually with two tables here:

  • The wallet table
  • The checkpoint table

That migration does two things:

  1. It adds the genesis hash and genesis start to the wallet table.
  2. It removes all the protocol parameters from the checkpoint table.

Thus, once removed from the checkpoint table isFieldPresentByName conn "checkpoint" "genesis_hash" would prevent the migration from re-running again.

Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

@@ -448,12 +451,15 @@ migrateManually tr proxy defaultFieldValues =
, "slot_length", "epoch_length", "tx_max_size"
, "epoch_stability", "active_slot_coeff"
]
_ <- runSql conn $ dropTable tmp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

let orig = "checkpoint"
let tmp = orig <> "_tmp"

-- 1. Add genesis_hash and genesis_start to the 'wallet' table.
[defaults] <- runSql conn $ select ["genesis_hash", "genesis_start"] orig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicitly, this assumes that is the genesis_hash field does not exists in the wallet table, then it necessarily exists in the checkpoint table. This should be true for quite many releases back and indeed, works fine with the way the migration is written 👍

@@ -479,7 +485,7 @@ migrateManually tr proxy defaultFieldValues =
]

dropTable table = mconcat
[ "DROP TABLE " <> table <> ";"
[ "DROP TABLE IF EXISTS " <> table <> ";"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 10, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit ffc9937 into master Feb 10, 2021
@iohk-bors iohk-bors bot deleted the rvl/adp-700/fix-checkpoint-db-migration branch February 10, 2021 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants