Document how to retire a whole table in the online-migration rules (PP-4653)

[dbernstein]

Description

Extends the "Online migrations (backwards compatibility)" section of CLAUDE.md to cover retiring a whole table. The section already explained the two-release split for columns (deferred() for reads, server_default for writes), but said nothing about tables, and the two rules that matter there are not obvious from the column rules.

Adds:

• 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). Leaving Parent.children mapped does not stop using the child table even when no application code touches the attribute.
• A fresh schema comes from the models, not the migrations. InstanceInitializationScript.initialize_database_schema calls SessionManager.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-commit clean (including the PyMarkdown hook). No code changes, so no test run applies.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 3m 18s —— View job

---

Summary

I checked the core guidance against the code and it holds up. InstanceInitializationScript.initialize_database_schema does run Base.metadata.create_all and then stamps head. The backwards-compat gate builds its "current" schema through initialize_instance. The external-schema test fixture clones that schema without walking Base.metadata, so keeping the model class in release 1 is safe in release 2's gate. The release-1 boundary between the relationship and the model is right, and so is moving the tests into release 1. The second commit's switch from TRUNCATE to DELETE fixes Greptile's lock concern. Two problems remain, both in the "Deleting the relationship can break parent deletes" bullet: emptying the table doesn't keep it empty while N-1 is still running, and the lock_timeout snippet carries over into later migrations.

Details

CLAUDE.md:198-200

Emptying the table in the release-1 migration doesn't keep it empty. The migration runs before the rollout, and N-1 still has the code that writes to this table, because release 1 is the first release that stops using it. So N-1 keeps inserting rows until its last pod shuts down. Any parent that gets a new child row in that window can't be deleted by release-1 code until release 2 drops the table: the foreign key rejects the delete and there's no relationship left to clean up. This is the same kind of N-1 breakage the section is meant to prevent. Only the ON DELETE clause covers rows written after the migration. Suggest making it the recommended fix, and offering the empty-the-table option only for tables that N-1 no longer writes to.

circulation/CLAUDE.md

Lines 198 to 200 in 6840c94

	without an `ON DELETE` clause will reject the parent delete while any row survives. Fix it either by
	giving the foreign key an `ON DELETE` clause, or by emptying the table in the release-1 migration — and if
	you empty it, use a plain `DELETE`, not `TRUNCATE`. The table is not quiet yet at that point: the

CLAUDE.md:206-207

SET lock_timeout lasts for the whole session. alembic/env.py runs every pending migration in one context.begin_transaction() on one connection (no transaction_per_migration). So the 2s timeout also applies to every migration that runs after this one in the same upgrade. A later, unrelated migration that normally waits a few seconds for a lock on a busy table would then fail and roll back the whole upgrade. This only matters when someone uses the TRUNCATE fallback and more migrations follow it in the same deploy, but the snippet will be copied as-is. Suggest resetting the timeout right after the TRUNCATE:

op.execute("SET lock_timeout = '2s'")
op.execute("TRUNCATE TABLE <table>")
op.execute("RESET lock_timeout")

circulation/CLAUDE.md

Lines 206 to 207 in 6840c94

	large for one `DELETE`, a `TRUNCATE` guarded by a short `lock_timeout`
	(`op.execute("SET lock_timeout = '2s'")` first) at least fails the migration fast instead of stalling

---

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; the previous blocking lock-contention guidance has been corrected, with only an existing non-blocking wording concern remaining.

Findings

1.  Relationship Reads Are Conditional ▶

Summary

This documentation-only PR expands the online-migration guidance for safely retiring an ORM-backed table across two releases.

• It places the compatibility boundary between removing ORM relationships and removing the model.
• It explains how model metadata determines freshly initialized schemas.
• It warns that relationship removal may change parent-delete behavior.
• Since the previous review, it replaces unsafe TRUNCATE guidance with ordinary DELETE or a fail-fast, timeout-guarded fallback.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Release 1: remove relationships and related tests] --> B[Keep model and table in fresh schemas]
  B --> C[Rolling deployment remains compatible with N-1]
  C --> D[Release 2: remove model]
  D --> E[Drop table through migration]

Loading

_{Reviews (2) · Last reviewed commit: "Correct the release-1 table cleanup guid..."}

[greptile-apps]

Relationship Reads Are Conditional

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.71%. Comparing base (7e44a87) to head (6840c94).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3745      +/-   ##
==========================================
+ Coverage   93.70%   93.71%   +0.01%     
==========================================
  Files         510      510              
  Lines       46487    46510      +23     
  Branches     6313     6313              
==========================================
+ Hits        43562    43589      +27     
+ Misses       1891     1887       -4     
  Partials     1034     1034              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.