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

summonerd: add new schema separating blob data #3305

Merged
merged 6 commits into from
Nov 9, 2023
Merged

summonerd: add new schema separating blob data #3305

merged 6 commits into from
Nov 9, 2023

Conversation

hdevalence
Copy link
Member

Towards #3304

@redshiftzero
fix: foreign key error due to order of tables
35fd73b 
phase1_contributions has a fk constraint to phase1_contribution_data
so it needs to be populated first
@redshiftzero
fix: transition query
ca2503c 
the transition query will need to populate phase_2_contribution_data
first in order for the FK constraint from phase2_contributions
to be valid
@redshiftzero redshiftzero marked this pull request as ready for review November 9, 2023 16:50
@redshiftzero
Copy link
Member

Summonerd smoke test is passing as of commit 4456f85, the migration process has been tested by:

  1. Checking out summonerd-feature-branch, starting a phase 1 ceremony locally and doing 1 phase 1 contribution. This results in two rows in the phase1_contributions table.
  2. Checking out commit 4456f85, and running the migration via:
sqlite3 /tmp/summonerd/ceremony.db < old-to-new-migration.sql
  1. Then starting the summoner again, verifying one can do a phase 1 contribution without issue ✅

@redshiftzero
Copy link
Member

redshiftzero commented Nov 9, 2023 •
edited
Loading

There was a smoke test failure here but it seems unrelated to the changes in this PR (subsequent run passed so confirmed is unrelated)

@cronokirby cronokirby self-requested a review November 9, 2023 17:12
@redshiftzero redshiftzero self-requested a review November 9, 2023 17:12
cronokirby
cronokirby approved these changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

I looked at storage.rs in isolation, I think every query was migrated; also, looking at the proposed query plans in sqlite, I think the queries all avoid scanning the blob table as well.

Migrations ran and passed locally.

@redshiftzero redshiftzero merged commit 35e1c9a into summonerd-feature-branch Nov 9, 2023
8 of 9 checks passed
@redshiftzero redshiftzero deleted the summonerd-schema-update branch November 9, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cronokirby cronokirby cronokirby approved these changes

@redshiftzero redshiftzero redshiftzero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hdevalence @redshiftzero @cronokirby