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: content_script_version_4.nim: migration failed when dropping unexisting constraint #2672

Merged
merged 4 commits into from
May 6, 2024

Conversation

Ivansete-status
Copy link
Collaborator

@Ivansete-status Ivansete-status commented May 6, 2024

Description

There appeared Postgres schema migration issue when going from schema version 3 to 4.

The problem occurred while trying to remove an index that doesn't seem to exist in version 3.

Aside from that, I'm adding the following line, which is part of the script for version 3, to make sure that index is created in case it doesn't exist. I've notice that when testing in waku.test:
CREATE INDEX IF NOT EXISTS i_query ON messages (contentTopic, pubsubTopic, storedAt, id);

@SionoiS
Copy link
Contributor

SionoiS commented May 6, 2024

I don't understand. The logs seams to suggest that the error happen because messages table already existed.

I don't see how this PR would fix that?

There's 4 cases here, messages and messages_backup both can either exist or not. If we don't handle all cases then it can fail.

@NagyZoltanPeter
Copy link
Contributor

I don't understand. The logs seams to suggest that the error happen because messages table already existed.

I don't see how this PR would fix that?

There's 4 cases here, messages and messages_backup both can either exist or not. If we don't handle all cases then it can fail.

Yeah, I agree with SP, the issue here that the migration failed at a stage when both table existed. Dropping the constraint will not solve this.
Or do you think the original issue with the migration happened because the existed constraint?

@Ivansete-status
Copy link
Collaborator Author

Yes sorry for the confusion guys. I will unset you from reviewers until I have a better fix.
Thanks for the comments!

Copy link

github-actions bot commented May 6, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2672-rln-v1

Built from ebcd370

Copy link

github-actions bot commented May 6, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2672-rln-v2

Built from ebcd370

Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Based on your description of the problem this seams good.

Thanks

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much! 😍

@Ivansete-status Ivansete-status merged commit 38f8b08 into master May 6, 2024
12 checks passed
@Ivansete-status Ivansete-status deleted the fix-migration-issue branch May 6, 2024 16:22
Ivansete-status added a commit that referenced this pull request May 14, 2024
@Ivansete-status Ivansete-status changed the title fix: content_script_version_4.nim: migration failed when dropping unexisting constraing fix: content_script_version_4.nim: migration failed when dropping unexisting constraint Aug 20, 2024
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.

4 participants