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

SQLite: move migration into into new() #517

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Dec 27, 2024

Description

This PR removes the need to call migrate() when a wallet or a mint uses SQLite.

This is achieved with the same approach used in redb, where migration is handled inside new().


Notes to the reviewers


Suggested CHANGELOG Updates

CHANGED

Simplify SQLite integration: remove need to call migrate()

ADDED

REMOVED

FIXED


Checklist

@ok300 ok300 force-pushed the ok300-simplify-sqlite branch from b3828cb to 16654b1 Compare December 27, 2024 15:49
@thesimplekid
Copy link
Collaborator

I've actually thought about going the other way with this and changing the redb to be in line with the sql one and force migrate to be called explicitly. My reasoning for this is it gives the user better control over db changes and allows them to create a backup before calling migrate. The other way we could handle this is create a backup before the migration, but do think we want to encourage or force a db backup before a migration.

related: #400

@ok300
Copy link
Contributor Author

ok300 commented Dec 30, 2024

we want to encourage or force a db backup before a migration

Agree.

If we give control to the caller over when to call migrate, then we must also give them control over backup / restore. Trickier IMO is when they have to make sure to call backup then migrate in the right order and at the right place after an upgrade. Forgetting to call migrate after an upgrade could lead to weird problems.

A simpler and more robust way is IMO to encapsulate this in the DB initialization:

  • create a backup on every startup (up to e.g. 10 backups)
  • call migrate

Then we can offer the methods

  • list_backups, which would list the available backups (incl. the timestamp and CDK version used for each)
  • restore, which the caller could offer in an Advanced view in their app if they wish

What do you think?

@thesimplekid
Copy link
Collaborator

Yes, you're right we should do it in the the DB initialization and create backups then. For the wallet especially I don't think we should create a backup each time as it uses more storage then is really needed but we can check if a migration is needed and back up then. For the mint a configurable number of backups should be fine.

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.

2 participants