-
Notifications
You must be signed in to change notification settings - Fork 0
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
Do in batches suggestion. #2
Conversation
@@ -74,7 +79,7 @@ func (db *BoltDB) upgradeDB() error { | |||
return nil | |||
} | |||
|
|||
db.log.Infof("Upgrading database from version %d to %d", version, DBVersion) | |||
fmt.Printf("Upgrading database from version %d to %d\n", version, DBVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Srry, got sloppy with the logs. Will fix if looks like something you can use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks great. I just wonder if we can tolerate a rollback that will leave an update partially completed. I'd argue this (a) has little potential to error if the first Update chunk succeeded and (b) we have a backup file created immediately prior that we can use.
This looks like a good solution to me anyway. May merge this in to the dcrdex PR and tweak a few thing like the logs.
@JoeGruffins I pushed another commit to the At this point though, I'm perplexed by the need for the Anyway, with your PR rebased on the updated upstream PR:
profile:
I'm just still unsure about the safety of upgrading in separate batches without ability to either roll all the way back or resume the upgrade if it gets interrupted. |
fmt.Printf("Updated %d entries (%d total)\n", numUpdated, totalUpdated) | ||
continue | ||
} | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we hit here, that means the user has to restore their db backup right? Perhaps a message.
I've since pushed a second commit, so yet more conflicts probably. We're trying 1070 on a big DB to see if we need to take make the batch changes as in this PR. If we do go this route, I think we'll at least need clear recovery messages about the backup db, an automatic restore of the backup file, or more sophisticated upgrade resumption like Dave described in the dexdev chan. I hope none of it is necessary though because it'll get hairy. Thanks for looking into this issue @JoeGruffins! EDIT: I also had to rebase 1070 on release-v0.2 for some easier testing. |
Feel free to close if not going to use. I don't know exactly how bolt works, but if all changes for a tx are saved in memory and done at once, or changes are done incrementally and the original is saved for rollback, it would make sense that txs with a lot of changes would use a lot of memory, and doing everything with one transaction will oom at some point. I really don't know though. |
I really don't know either. We may very well need this approach. Let's just hang on to it for now. Reports are the most challenging DBs so far have managed the upgrade with the upstream changes, but clearly we have scaling issues. Thanks for this work. |
61190fd
to
bbd3bfd
Compare
Upstream branch got deleted so this PR auto-closed, but this is still on the table. |
No description provided.