fix: try to prevent deadlocks when running upgrade #29625
Conversation
| constraint = fk["name"] | ||
| if constraint: | ||
| op.execute( | ||
| f"ALTER TABLE {table} RENAME CONSTRAINT {constraint} TO {constraint}_old" |
There was a problem hiding this comment.
This may have helped in my earlier PR when we were changing the constraint, but after looking into this, I don't think it will help when dropping the constraint. Maybe one idea would be to drop each table in a separate transaction and commit after each drop.
There was a problem hiding this comment.
careful a commit may leave a migration partially applied
There was a problem hiding this comment.
ok, I revised this to just drop the constraint first and then afterwards to drop the table. Hopefully this will reduce any deadlocks there may be.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29625 +/- ##
===========================================
+ Coverage 60.48% 83.70% +23.21%
===========================================
Files 1931 527 -1404
Lines 76236 38069 -38167
Branches 8568 0 -8568
===========================================
- Hits 46114 31866 -14248
+ Misses 28017 6203 -21814
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| "sl_datasets" | ||
| ] | ||
|
|
||
| def drop_constraints(table, insp): |
There was a problem hiding this comment.
This function seems useful and could be moved into superset/migrations/shared/utils.py so it can be reused in future migrations. I'd make both insp and conn arguments so the caller can reuse theirs and control transations.
|
Just a note, but in light of #29546, thinking in the future this type of migration would have been held up until the next major. In the meantime, it's probably ok to mitigate existing migrations to minimize deadlock/downtime. |
sadpandajoe
force-pushed
the
joe/prevent-postgres-deadlock-for-remove-sl_tables
branch
from
d61b0df
to
b394083
Compare
sadpandajoe
added
the
v4.1
sadpandajoe
deleted the
joe/prevent-postgres-deadlock-for-remove-sl_tables
branch
SUMMARY
When running the migration upgrade path for #28704, we noticed that there could be times where we see a deadlock. This PR breaks down each drop into it's own migration file.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION