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

fix: avoid double db migration for sqlite #3244

Merged
merged 4 commits into from
Jan 20, 2025

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Jan 17, 2025

Description

Avoiding double db migration when both Store and Legacy Store are mounted and sqlite is used.

When both store protocols are mounted, two db migrations are performed: one according to Legacy Store's migration definition and then other according to the newer Store. At the end, we end up having the DB schema set by the newer store.

However, in sqlite the schemas of Store and Legacy Store are different (Legacy Store has a storedAt field).Therefore, Legacy Store can't work properly with Store's schema.

Given that Legacy Store is planned to be deprecated and removed soon, instead of creating new migrations and making significant changes in the legacy archive driver to stop using the storedAt field, I'm introducing a pretty dirty workaround so we make sure that we don't get to a point of using a db with an incompatible schema.

The idea is to remove this piece of code as soon as we remove sqlite's Legacy Store. If the plan is to support Legacy Store for a long time, then this workaround is terrible and we should adapt sqlite's Legacy Store to stop using the storedAt field, the same way it's done in Postgres' Legacy Store.

However, if the plan is to remove Legacy Store soon, then this dirty workaround will spare us from lots of unnecessary work.

Changes

  • avoid performing Store migration if Legacy Store is enabled and sqlite is being used

Issue

#3236

Copy link

github-actions bot commented Jan 17, 2025

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3244

Built from 156127a

@gabrielmer gabrielmer changed the title fix: avoid double db migration when both store and legacy store are m… fix: avoid double db migration Jan 17, 2025
@gabrielmer gabrielmer changed the title fix: avoid double db migration fix: avoid double db migration for sqlite Jan 17, 2025
@gabrielmer gabrielmer marked this pull request as ready for review January 17, 2025 16:46
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM as long as the test CI pass :)

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Seems ok, just have had a question out of my limited knowledge on the topic.

@gabrielmer gabrielmer merged commit 2ce2453 into master Jan 20, 2025
10 of 11 checks passed
@gabrielmer gabrielmer deleted the fix-avoid-double-db-migration branch January 20, 2025 15:13
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