fix(BA-6913): reconcile indexes that only exist on migrated databases#12907
Draft
jopemachine wants to merge 5 commits into
Draft
fix(BA-6913): reconcile indexes that only exist on migrated databases#12907jopemachine wants to merge 5 commits into
jopemachine wants to merge 5 commits into
Conversation
The schema is defined twice: a fresh install builds tables from the model metadata via `mgr schema oneshot` and stamps head without running a single migration, while an existing install replays the migration files. Nothing checks that the two agree, and for three indexes they did not -- the migrations create them, the models never declared them, so only migrated databases ever had them. The worst of the three covers role_invitations.invitee_user_id, whose FK to users is ON DELETE CASCADE: without it a fresh install scans the table on every user deletion. uq_role_invitations_active cannot stand in for it because it is partial (state != 'accepted') and so misses exactly the rows a cascade has to find. Declare all three on the models, and add a migration so databases already deployed by oneshot pick them up; IF NOT EXISTS keeps it a no-op where they are already present. Also drop the redundant drop_index calls that made this visible: the following drop_column/drop_table removes the index anyway, so naming it first only broke downgrades on installs that never had it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Converging the two schema paths only makes sense in the direction the index earns. role_invitations.invitee_user_id and prometheus_query_presets.category_id each back a foreign key whose parent-side delete (CASCADE / SET NULL) has to find the referencing rows, so declaring them costs a write and saves a scan. vfolders.creator_id has neither: no foreign key, and nothing in the tree filters, joins, or orders by it -- it is only ever written and selected. Declaring it would add write cost to every vfolder for no reader, so leave the model as it was. Whether the migration should have created that index at all is a separate question from this one. The redundant drop_index removal in b4e7f1a2c3d5 stays: drop_column takes the index with it whether or not the index is there, so that downgrade passes on a fresh install either way. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Converging the two schema paths only makes sense in the direction the index earns. vfolders.creator_id carries no foreign key and is never filtered, joined, or ordered by anywhere in the tree -- it is only written and selected -- so the index it got in b4e7f1a2c3d5 costs a write on every vfolder and saves nothing. Rather than declare it on the model to match, stop creating it, and drop it from the databases that already ran that revision. The reconcile migration now moves each index in whichever direction it earns: it creates the two that back a foreign key whose parent-side delete must find the referencing rows, and drops this one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📚 Stacked PRs
BA-6913fix: indexes that only exist on migrated databases ← you are hereBA-6914fix: one FK constraint name on both install pathsBA-6915fix: drop the column default before retyping sessions.resultMerge in order, bottom-up. Each PR is based on the one above it, so its diff shows only its own layer.
Problem
The schema is defined twice, and nothing checks that the two agree:
mgr schema oneshotalembic upgrade headcreate_all→ stamp headThree migrations create an index the matching model never declared, so it exists only on migrated databases. Verified on a DB built at head by
oneshot: none of the three is present, and no later migration drops them.Convergence has two directions
Which way each index goes depends on whether it earns its write cost, so each was checked individually rather than fixed by pattern:
role_invitations.invitee_user_idON DELETE CASCADEprometheus_query_presets.category_idON DELETE SET NULLvfolders.creator_idsrc/ai/backend/role_invitations.invitee_user_idis the one that bites: on a fresh install every user deletion scansrole_invitations.uq_role_invitations_activecannot substitute — it is partial (WHERE state != 'accepted'), so it excludes exactly the rows a cascade must find.vfolders.creator_idgoes the other way. Nothing reads it, so the indexb4e7f1a2c3d5created costs a write on every vfolder and saves nothing. Declaring it on the model to match would have spread the cost to every install; instead its creation is removed and migrated databases have it dropped.Fix
index=Truereproduces the migrations' exactix_<table>_<column>names).create_indexfromb4e7f1a2c3d5.b2f7c1a94e30to reconcile databases already out there — creates the two, drops the one. Existence checks make every statement a no-op where the DB already agrees; its downgrade is a no-op, since undoing it would only put the drift back.drop_indexcalls in all three: the followingdrop_column/drop_tableremoves the index anyway, so naming it first only broke installs that never had it. This is independent of the model change —b4e7f1a2c3d5's downgrade passes on a fresh install with no such index.Verification
All against real databases; the dev DB was only ever read.
Fresh install — exactly the intended set: the two FK indexes present,
ix_vfolders_creator_idabsent.Old
oneshotinstall (schema built frommain's models, stamped at the old headaa27f1d5cd35):upgrade headMigrated DB carrying the useless index:
upgrade headRe-running after a downgrade produces no errors.
Edited
b4e7f1a2c3d5round-trip: downgrade dropscreator_id; re-upgrade restores the column and does not create the index.Downgrade on a fresh install (stamp the revision, run only its downgrade):
mainad7acfe8aa1cindex "ix_role_invitations_invitee_user_id" does not existb4e7f1a2c3d5index "ix_vfolders_creator_id" does not exista3b4c5d6e7f8index "ix_prometheus_query_presets_category_id" does not exist¹ Honest result: clearing the index error on
a3b4c5d6e7f8exposes a second, different defect underneath — its downgrade also dropsfk_prometheus_query_presets_category_id, but a fresh install names that constraint by the metadata convention, which exceeds PostgreSQL's 63-char identifier limit and comes out truncated and hash-suffixed asfk_prometheus_query_presets_category_id_prometheus_quer_8a51. That is the FK-naming drift class (BA-6914, also affectscd067180a8b1andc7f2a8e31b04) — those constraints are enforced on both paths, only the names differ. This PR does not claim to fix it.Found while surveying the downgrade chain for #12881.
🤖 Generated with Claude Code