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: add missing columns #13216

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Feb 18, 2021

SUMMARY

The migration script b56500de1855_add_uuid_column_to_import_mixin.py has a try/except block when adding the uuid column to tables; if the operation fails the script might succeed depending on the error. Because of this, people who have ran the migration are having problems running Superset because of missing columns.

I fixed the script by removing the try/except block, and adding a new script that adds the column wherever necessary.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

  1. Ran migration, nothing happened as expected (since all my tables have the uuid column)
  2. Downgraded to 070c043f2fdb and deleted the uuid column from the dashboards table
  3. Ran migration, and verified that uuid was added to dashboards and populated.

ADDITIONAL INFORMATION

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM. Was this user-reported? If so, do you mind linking the GitHub issue if it exists?

I think another way to do this is do the column check in add_uuid_column_to_import_mixin.py, then simply re-run the same upgrade again in add_missing_uuid_column.py. This way you avoid the maximum amount of duplicate code, but I guess current approach works, too.

@betodealmeida
Copy link
Member Author

betodealmeida commented Feb 18, 2021

LGTM. Was this user-reported? If so, do you mind linking the GitHub issue if it exists?

Good call. I saw the reports on Slack and Stackoverflow, but there's also a ticket which I just added.

I think another way to do this is do the column check in add_uuid_column_to_import_mixin.py, then simply re-run the same upgrade again in add_missing_uuid_column.py. This way you avoid the maximum amount of duplicate code, but I guess current approach works, too.

The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.

@ktmud
Copy link
Member

ktmud commented Feb 18, 2021

The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.

I was proposing adding the new script, but it rerun the modified code that ignores existing uuid columns. Users don't have to downgrade because it's OK to rerun the same upgrade script when it can ignore existing uuid columns.

@mistercrunch mistercrunch added the risk:db-migration PRs that require a DB migration label Feb 18, 2021
@betodealmeida
Copy link
Member Author

The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.

I was proposing adding the new script, but it rerun the modified code that ignores existing uuid columns. Users don't have to downgrade because it's OK to rerun the same upgrade script when it can ignore existing uuid columns.

Ah, that makes sense. Sorry I misunderstood.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

This LGTM.

People do tend to get tangled in "partially executed database migrations", where say if the constraint creation fails (step 2), then step 1 won't run anymore on re-attempts. Then you have people reporting the stack trace from step one failing "column already exist" on subsequent execution, which is a red herring. Solution is to either go in and delete the column in the database manually, or commenting step 1 in the source code, both of which are super confusing for people not super familiar with this type of issues.

Eventually we could have more robust migrations that tend to fall back on their feet more automatically (check if column exists, ...). I don't think that's in-scope for this PR but wanted to point this out.

@betodealmeida betodealmeida merged commit b34c863 into apache:master Feb 18, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels risk:db-migration PRs that require a DB migration size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants