Skip to content

Conversation

@yuvmen
Copy link
Member

@yuvmen yuvmen commented Nov 18, 2025

These two models were already preivously deleted and a previous migration dropped them, but due to an issue with SafeDeleteModel they were not actually dropped. This migration drops them directly.

…cription` leftover tables

These two models were already preivously deleted and a previous migration dropped them, but due to
an issue with SafeDeleteModel they were not actually dropped. This migration drops them directly.
@yuvmen yuvmen requested a review from a team as a code owner November 18, 2025 22:05
@yuvmen yuvmen requested a review from wedamija November 18, 2025 22:05
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 18, 2025
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/1004_drop_legacy_incidentseen_incidentsubscription.py

for 1004_drop_legacy_incidentseen_incidentsubscription in sentry

--
-- Raw SQL operation
--

            DROP TABLE IF EXISTS sentry_incidentseen;
            
--
-- Raw SQL operation
--

            DROP TABLE IF EXISTS sentry_incidentsubscription;

Copy link
Member

@vgrozdanic vgrozdanic left a comment

Choose a reason for hiding this comment

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

Do we know why this PR #81623 didn't drop the tables?

operations = [
SafeRunSQL(
"""
DROP TABLE IF EXISTS sentry_incidentseen;
Copy link
Member

Choose a reason for hiding this comment

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

Should we also CASCADE deletes?

Not sure how was this used before, but previous PR that for to me unknown reason did't work generated following SQL:

--
-- Delete model IncidentSeen
--
DROP TABLE "sentry_incidentseen" CASCADE;
--
-- Delete model IncidentSubscription
--
DROP TABLE "sentry_incidentsubscription" CASCADE;

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm yea it seemed wrong to cascade but now I realize the original had it, should probably be okay to cascade here as well - ill add it

# is a schema change, it's completely safe to run the operation after the code has deployed.
# Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment

is_post_deployment = True
Copy link
Member

Choose a reason for hiding this comment

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

If there are no cascading drops - this can be in-deployment migration, since the drop is quite fast when there are no active transactions on the table

@markstory
Copy link
Member

Do we know why this PR #81623 didn't drop the tables?

This could be related to database routing not being able to route the table to a database. When the model class is removed, the silo assignment of the table is also lost. Adding these table names to the historical_silo_assignments could help.

historical_silo_assignments = {
"authidentity_duplicate": SiloMode.CONTROL,
"authprovider_duplicate": SiloMode.CONTROL,
"sentry_actor": SiloMode.REGION,
"sentry_alertruleactivations": SiloMode.REGION,
"sentry_monitorlocation": SiloMode.REGION,
"sentry_notificationsetting": SiloMode.CONTROL,
"sentry_pagerdutyservice": SiloMode.REGION,
"sentry_projectavatar": SiloMode.REGION,
"sentry_scheduledjob": SiloMode.CONTROL,
"sentry_teamavatar": SiloMode.REGION,
"workflow_engine_workflowaction": SiloMode.REGION,
}

I suspect that you'll need to update the historical_silo_assignments to get this migration to run correctly as well.

@yuvmen
Copy link
Member Author

yuvmen commented Nov 19, 2025

@markstory - yea @wedamija and I discussed it, he didnt seem to believe it was necessary anymore and that SafeRunSQL should work without, though we agree thats probably why it failed originally.
theres actually a problem there where if it fails on that it just does a no-op silently because of this if:

if self.allow_migrate_model(schema_editor.connection.alias, model):
schema_editor.delete_model(model, is_safe=True)

@wedamija is looking into preventing this in the future I believe

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #103559      +/-   ##
===========================================
+ Coverage   75.42%    80.59%   +5.17%     
===========================================
  Files        9260      9259       -1     
  Lines      395305    395356      +51     
  Branches    25213     25179      -34     
===========================================
+ Hits       298160    318642   +20482     
+ Misses      96716     76284   -20432     
- Partials      429       430       +1     

@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/1006_drop_legacy_incidentseen_incidentsubscription.py

for 1006_drop_legacy_incidentseen_incidentsubscription in sentry

--
-- Raw SQL operation
--

            DROP TABLE IF EXISTS sentry_incidentseen CASCADE;
            
--
-- Raw SQL operation
--

            DROP TABLE IF EXISTS sentry_incidentsubscription CASCADE;

@yuvmen yuvmen merged commit ad6d497 into master Nov 19, 2025
66 of 67 checks passed
@yuvmen yuvmen deleted the yuvmen/manually-drop-legacy-incident-tables branch November 19, 2025 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants