Document how to retire a whole table in the online-migration rules (PP-4653) - #3745
dbernstein wants to merge 1 commit into
Conversation
The online-migration section explained the two-release split for columns (deferred, server_default) but not for tables, and the missing guidance is what let PR #3521 reach review with a drop that would have broken N-1 webservers. Two traps were undocumented: a relationship() is a read, because SQLAlchemy loads it on every parent delete to cascade or to null the child's foreign key; and a fresh database's schema comes from create_all over the models rather than from replaying migrations, so deleting a model class removes its table from new installs regardless of the migration. Together they put the release boundary between the relationship and the model, not around the model. Also note the two follow-on details that bit us: removing the relationship makes surviving rows block parent deletes, and N-1's test suite must not contain tests that write to the doomed table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @dbernstein's task in 4m 24s —— View job SummaryGood, well-sourced addition — I verified the two load-bearing claims against the tree and both hold: Details
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| in the release-1 migration (right when the rows are dead data — `TRUNCATE` avoids the row-level WAL of a | ||
| large `DELETE`, and its `ACCESS EXCLUSIVE` lock is uncontended on a table nothing reads) or give the | ||
| foreign key an `ON DELETE` clause. A FK that already declares `ON DELETE CASCADE` needs neither. |
There was a problem hiding this comment.
The release-1 migration runs while N-1 webservers are still active, and those servers retain the relationships that this section says query the child table during parent deletion. Therefore, TRUNCATE may wait behind existing transactions or block those requests because it requires an ACCESS EXCLUSIVE lock. Describing that lock as “uncontended” could lead maintainers to turn an online migration into a deployment stall; this guidance should account for N-1 traffic and offer a lock-safe cleanup strategy.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| - **A `relationship()` is a read.** SQLAlchemy loads a relationship whenever its parent is deleted — to | ||
| cascade the delete (`mapper.cascade_iterator`), or to null the child's foreign key | ||
| (`dependency.presort_deletes`). So leaving `Parent.children` mapped does **not** stop using the child | ||
| table, even when no application code ever touches the attribute: every `session.delete(parent)` still | ||
| SELECTs from it. Release 1 has to delete the `relationship()` definitions themselves, and the matching | ||
| `back_populates` on the other side — retiring the code that *used* them is not enough. | ||
| - **A fresh schema is built from the models, not by replaying migrations.** |
There was a problem hiding this comment.
The statement that SQLAlchemy loads a relationship whenever its parent is deleted is too broad. This repository has relationships configured with passive_deletes=True, where database-side deletion can avoid loading unloaded children. Please narrow the explanation to relationships whose cascade or nulling behavior requires ORM participation; otherwise, maintainers may make unnecessary changes when retiring a table.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3745 +/- ##
==========================================
- Coverage 93.70% 93.70% -0.01%
==========================================
Files 510 510
Lines 46487 46487
Branches 6313 6313
==========================================
- Hits 43562 43561 -1
- Misses 1891 1892 +1
Partials 1034 1034 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Description
Extends the "Online migrations (backwards compatibility)" section of
CLAUDE.mdto cover retiring a whole table. The section already explained the two-release split for columns (deferred()for reads,server_defaultfor writes), but said nothing about tables, and the two rules that matter there are not obvious from the column rules.Adds:
relationship()is a read. SQLAlchemy loads a relationship whenever its parent is deleted — to cascade the delete (mapper.cascade_iterator) or to null the child's foreign key (dependency.presort_deletes). LeavingParent.childrenmapped does not stop using the child table even when no application code touches the attribute.InstanceInitializationScript.initialize_database_schemacallsSessionManager.initialize_schema(metadata.create_all) and stamps alembic head, so deleting a model class removes its table from newly initialized databases immediately — and the backwards-compatibility gate builds its "current" schema exactly this way.Together these place the release boundary between the relationship and the model, which the new text spells out as a numbered sequence, plus two follow-on details: removing a relationship can make surviving rows block parent deletes (the relationship was what cascaded by hand), and the model's tests have to go in release 1 because the gate runs N-1's test suite against the new schema.
Documentation only — no code changes.
Motivation and Context
JIRA (PP-4653)
This is the guidance whose absence let #3521 reach review with a table drop that would have broken N-1 webservers during a rolling deploy. #3520 followed the documented rule — "stop using the object in the code, but leave it in the schema" — and still left three live ORM reads behind, because the rule as written only talks about columns.
Writing it down so the next table retirement starts from the right split instead of rediscovering it from a red backwards-compatibility gate.
How Has This Been Tested?
pre-commitclean (including the PyMarkdown hook). No code changes, so no test run applies.Checklist
🤖 Generated with Claude Code