Fix pool registration rollback #1201
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paweljakubas
approved these changes
Dec 18, 2019
There was a problem hiding this comment.
PR is remedy for
SQLite3 returned ErrorConstraint while attempting to perform step: UNIQUE constraint failed: pool_owner.pool_id, pool_owner.pool_owner
problem and more.
- It makes pool registration dependent on slot id, and not epoch number, which is important for rollbacks - and produced db problem ^^^. It would be enough to solve this problem BUT would require user intervention which was decided to be avoided. So, we have point 2.
- It solves it potential user engagement in solving manually db migration (without bothering user) by removing db related files and instantiating again db backend. So maybe there are going to be other cases than the one illustrated in point 1 where avoiding manual db intervention is needed and hence is goes beyond solving this particular problem
| describe "Migration Regressions" $ do | ||
| test_migrationFromv20191216 | ||
|
|
||
| test_migrationFromv20191216 :: Spec |
| @@ -205,6 +208,9 @@ instance ToText DBLog where | |||
| MsgQuery stmt _ -> stmt | |||
| MsgConnStr connStr -> "Using connection string: " <> connStr | |||
| MsgClosing fp -> "Closing database ("+|fromMaybe "in-memory" fp|+")" | |||
| MsgDatabaseReset -> | |||
…lem. It now fails
… rollback! This is however a DB breaking change which requires migration... Yet, there's no way to migrate from an existing database to this new schema. Since however this is a preferable solution and, re-syncing the pools data is cheap, we'll simply discards any existing database that has an invalid format and, resync from scratch :)
…sn't work out of the box Currently fails with: ``` PersistError "\n\nDatabase migration: manual intervention required.\nThe unsafe actions are prefixed by '***' below:\n\n CREATE TEMP TABLE \"pool_registration_backup\"(\"pool_id\" VARCHAR NOT NULL,\"slot\" INTEGER NOT NULL,\"margin\" INTEGER NOT NULL,\"cost\" INTEGER NOT NULL, PRIMARY KEY (\"pool_id\"));\n INSERT INTO \"pool_registration_backup\"(\"pool_id\",\"margin\",\"cost\") SELECT \"pool_id\",\"margin\",\"cost\" FROM \"pool_registration\";\n*** DROP TABLE \"pool_registration\";\n CREATE TABLE \"pool_registration\"(\"pool_id\" VARCHAR NOT NULL,\"slot\" INTEGER NOT NULL,\"margin\" INTEGER NOT NULL,\"cost\" INTEGER NOT NULL, PRIMARY KEY (\"pool_id\"));\n INSERT INTO \"pool_registration\" SELECT \"pool_id\",\"slot\",\"margin\",\"cost\" FROM \"pool_registration_backup\";\n DROP TABLE \"pool_registration_backup\";\n" ```
As described in the previous commit.
KtorZ
force-pushed
the
KtorZ/1200/fix-rollback-registration
branch
from
112927b
to
964f464
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Dec 18, 2019
1201: Fix pool registration rollback r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1200 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - commit 18852c0 modify rollback property to capture the rollback in-within epoch problem. It now fails - commit 3a4d4fd store registration with full slot and not only epoch to allow correct rollback! This is however a DB breaking change which requires migration... Yet, there's no way to migrate from an existing database to this new schema. Since however this is a preferable solution and, re-syncing the pools data is cheap, we'll simply discards any existing database that has an invalid format and, resync from scratch :) - commit f21f4e3 add a regression test to show that migration from an old database doesn't work out of the box Currently fails with: ``` PersistError Database migration: manual intervention required. The unsafe actions are prefixed by '***' below: CREATE TEMP TABLE \"pool_registration_backup\"(\"pool_id\" VARCHAR NOT NULL,\"slot\" INTEGER NOT NULL,\"margin\" INTEGER NOT NULL,\"cost\" INTEGER NOT NULL, PRIMARY KEY (\"pool_id\")); INSERT INTO \"pool_registration_backup\"(\"pool_id\",\"margin\",\"cost\") SELECT \"pool_id\",\"margin\",\"cost\" FROM \"pool_registration\"; *** DROP TABLE \"pool_registration\"; CREATE TABLE \"pool_registration\"(\"pool_id\" VARCHAR NOT NULL,\"slot\" INTEGER NOT NULL,\"margin\" INTEGER NOT NULL,\"cost\" INTEGER NOT NULL, PRIMARY KEY (\"pool_id\")); INSERT INTO \"pool_registration\" SELECT \"pool_id\",\"slot\",\"margin\",\"cost\" FROM \"pool_registration_backup\"; DROP TABLE \"pool_registration_backup\";" ``` - commit 440e099 handle migration error in stake pool db layer As described in the previous commit. - commit 112927b better assertions on logs message for db migration tests # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build failed |
KtorZ
force-pushed
the
KtorZ/1200/fix-rollback-registration
branch
from
964f464
to
ce50fb0
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Dec 18, 2019
1201: Fix pool registration rollback r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1200 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - commit 18852c0 modify rollback property to capture the rollback in-within epoch problem. It now fails - commit 3a4d4fd store registration with full slot and not only epoch to allow correct rollback! This is however a DB breaking change which requires migration... Yet, there's no way to migrate from an existing database to this new schema. Since however this is a preferable solution and, re-syncing the pools data is cheap, we'll simply discards any existing database that has an invalid format and, resync from scratch :) - commit f21f4e3 add a regression test to show that migration from an old database doesn't work out of the box Currently fails with: ``` PersistError Database migration: manual intervention required. The unsafe actions are prefixed by '***' below: CREATE TEMP TABLE \"pool_registration_backup\"(\"pool_id\" VARCHAR NOT NULL,\"slot\" INTEGER NOT NULL,\"margin\" INTEGER NOT NULL,\"cost\" INTEGER NOT NULL, PRIMARY KEY (\"pool_id\")); INSERT INTO \"pool_registration_backup\"(\"pool_id\",\"margin\",\"cost\") SELECT \"pool_id\",\"margin\",\"cost\" FROM \"pool_registration\"; *** DROP TABLE \"pool_registration\"; CREATE TABLE \"pool_registration\"(\"pool_id\" VARCHAR NOT NULL,\"slot\" INTEGER NOT NULL,\"margin\" INTEGER NOT NULL,\"cost\" INTEGER NOT NULL, PRIMARY KEY (\"pool_id\")); INSERT INTO \"pool_registration\" SELECT \"pool_id\",\"slot\",\"margin\",\"cost\" FROM \"pool_registration_backup\"; DROP TABLE \"pool_registration_backup\";" ``` - commit 440e099 handle migration error in stake pool db layer As described in the previous commit. - commit 112927b better assertions on logs message for db migration tests # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build failed |
|
Nix failed on stylish haskell job 🤷♂️ ... re-running manually... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
#1200
Overview
commit 18852c0
modify rollback property to capture the rollback in-within epoch problem. It now fails
commit 3a4d4fd
store registration with full slot and not only epoch to allow correct rollback!
This is however a DB breaking change which requires migration...
Yet, there's no way to migrate from an existing database to this new
schema. Since however this is a preferable solution and, re-syncing
the pools data is cheap, we'll simply discards any existing database
that has an invalid format and, resync from scratch :)
commit f21f4e3
add a regression test to show that migration from an old database doesn't work out of the box
Currently fails with:
commit 440e099
handle migration error in stake pool db layer
As described in the previous commit.
commit 112927b
better assertions on logs message for db migration tests
Comments