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.

Sign up for GitHub

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

Jump to bottom

SQLite: Prepare schema for rollback #672

Merged
merged 1 commit into from
Sep 6, 2019
Merged

SQLite: Prepare schema for rollback #672

merged 1 commit into from
Sep 6, 2019

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Sep 2, 2019

Relates to #642

Overview

Makes changes to the SQL schema so that all entities can be rolled back.

The AD state tables are split into:

  • non-slot state.
  • state related to a slot.
  • state related to a checkpoint, which will be deleted when the checkpoint is purged.

Comments

Note that you will need to delete and recreate your wallet dbs after this commit.

@rvl rvl self-assigned this Sep 2, 2019
@rvl rvl mentioned this pull request Sep 2, 2019
Closed
jonathanknowles
jonathanknowles previously requested changes Sep 3, 2019
View reviewed changes
Copy link
Member

@jonathanknowles jonathanknowles left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @rvl.

I've had a quick look, and looks good so far, but I'll have another look tomorrow (to properly digest the PR). I've left some preliminary comments.

lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs Show resolved Hide resolved
Anviking
Anviking reviewed Sep 4, 2019
View reviewed changes
lib/core/src/Cardano/Wallet/DB/Sqlite.hs
deleteWhere [ SeqStateAddressesSeqStateId <-. ssid ]
deleteWhere [ SeqStatePendingIxSeqStateId <-. ssid ]
cpid <- selectList [ SeqStateCheckpointWalletId ==. wid ] []
deleteWhere [ SeqStatePendingIxCheckpointId <-. fmap entityKey cpid ]
Copy link
Member

@Anviking Anviking Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not directly relevant to this, but these are run as "separate SQL statements"?

Should we use something like Sqlite transactions, to protect against inconsistent state from the program crashing in the middle of a complex sequence of deletes/updates/inserts?

We do use DB.withLock from the WalletLayer, but I think that just protects against multiple wallets/requests mutating the DB at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed (as mentioned here: https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet.hs#L682-L685). The lock is only there to prevent concurrent issues in case clients are making concurrent conflicting requests, but it doesn't prevent data corruption / inconsistency like a transaction would do.

I recall @rvl had a reason for not using sqlite transaction here. I think it was something about being only partially supported by our driver library 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point.
It is running each DBLayer method in a transaction because we are using runSqlConn.
At the time of writing the initial DBLayer we were a bit unsure where to draw the lines with transactions and locking.
We ought to have another look. In particular, see if we can put a transaction around restoreBlocks in the wallet layer.

@Anviking
Copy link
Member

Anviking commented Sep 4, 2019

restore bench seems very fast now (again) 👍

  restore testnet seq: 6.066 s
  restore testnet 10% ownership: 6.065 s
Benchmark restore: FINISH

Similar to what I got on 12908ba, and lower than on master (over 100-200s).

I think I recall usually seeing around 11s in the past, but not sure. (Btw, we can't plot historical values from buildkite?)

@rvl
SQLite: Prepare schema for rollback
218bdfd 
Note that you will need to delete and recreate your wallet dbs after
this commit.
@rvl rvl force-pushed the rvl/642/sqlite-schema-rollback branch from 54ab0a0 to 218bdfd Compare September 5, 2019 10:48
@rvl
Copy link
Contributor Author

rvl commented Sep 6, 2019

@Anviking About benchmarking.

  1. For evaluating DB performance, the DB benchmarks will be more precise at measuring specific operations than the restore benchmark.
  2. I have made sure that the benchmark results are saved as a buildkite artifact for every run. The criterion results are saved as json and the restore results are saved as text. So we could plot historical results with a little bit of effort. But they won't be reliable measures because sometimes the build machines change. It would be better to build revisions locally and compare. The nix build is helpful for this because it's reproducible.

KtorZ
KtorZ approved these changes Sep 6, 2019
View reviewed changes
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.

LGTM 👍

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 6, 2019

Already running a review

iohk-bors bot added a commit that referenced this pull request Sep 6, 2019
@iohk-bors @rvl @jonathanknowles
Merge #672 #680
28fb9c1 
672: SQLite: Prepare schema for rollback r=KtorZ a=rvl

Relates to #642 

# Overview

Makes changes to the SQL schema so that all entities can be rolled back.

The AD state tables are split into:
- non-slot state.
- state related to a slot.
- state related to a checkpoint, which will be deleted when the checkpoint is purged.

# Comments

Note that you will need to delete and recreate your wallet dbs after this commit.


680: Fix documentation comments in `Cardano.Wallet`. r=KtorZ a=jonathanknowles

# Issue Number

None.

# Overview

Fixes some typos and removes redundant comments from module `Cardano.Wallet`.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 6, 2019

Build succeeded

@iohk-bors iohk-bors bot merged commit 218bdfd into master Sep 6, 2019
@rvl rvl deleted the rvl/642/sqlite-schema-rollback branch September 6, 2019 22:46
@KtorZ KtorZ added this to the Support Rollbacks milestone Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanknowles jonathanknowles jonathanknowles left review comments

@Anviking Anviking Anviking left review comments

@KtorZ KtorZ KtorZ approved these changes

@paweljakubas paweljakubas paweljakubas approved these changes

Assignees

@rvl rvl

Labels
None yet
Projects
None yet
Milestone
Support Rollbacks
Development

Successfully merging this pull request may close these issues.
5 participants
@rvl @Anviking @jonathanknowles @KtorZ @paweljakubas