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

Migrate the db if it is based on a "broken" version of rev7 #299

Merged
merged 9 commits into from
May 24, 2022

Conversation

CHr15F0x
Copy link
Member

If the db schema contains ON DELETE CASCADE, then this migration is a noop.

@CHr15F0x CHr15F0x marked this pull request as ready for review May 23, 2022 10:08
@CHr15F0x CHr15F0x requested review from kkovaacs and koivunej May 23, 2022 10:08
Copy link
Contributor

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Looking good, rebase away. Still thinking about the usefulness of the general tests for migrations but the reproducer is good.

@CHr15F0x CHr15F0x force-pushed the CHr15F0x/fix_schema branch from d9e813f to 2a99b07 Compare May 23, 2022 10:39
@CHr15F0x CHr15F0x requested a review from koivunej May 23, 2022 14:55
- the virtual table `starknet_events_keys` remains unchanged
- but as it references rowids from the old `starknet_events` table
- we need to make sure that the recreated `starknet_events` table
  contains the same rowids
- otherwise `starknet_events_keys` could contain stale data and
  `starknet_getEvents` could cease to retrun valid replies
@CHr15F0x CHr15F0x force-pushed the CHr15F0x/fix_schema branch from fbd4179 to fa7a6e2 Compare May 24, 2022 07:47
Copy link
Contributor

@kkovaacs kkovaacs left a comment

Choose a reason for hiding this comment

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

LGTM!

@CHr15F0x CHr15F0x merged commit d3e2866 into main May 24, 2022
@CHr15F0x CHr15F0x deleted the CHr15F0x/fix_schema branch May 24, 2022 09:02
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