From 08418d54a3270bd0dea0e91430fda4ea0e059bdd Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 31 Aug 2026 08:25:41 -0400 Subject: [PATCH] db: index the seven unindexed FKs, drop the seven redundant ones (#3300, #3301) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A structural sweep of the deployed schema, run AFTER 0088 got the models and the chain to exact agreement. That agreement is what 0088 achieved, and it is worth naming what it does not prove: a models-vs-chain diff shows the two describe the same schema, not that the schema is right. Everything here was wrong in BOTH. The one that matters: image_tag has PRIMARY KEY (image_record_id, tag_id) and no other index, so tag_id is unindexed. That is the gallery's tag filter (tag_query.py builds `image_tag.c.tag_id == tid`) and the ON DELETE CASCADE from tag, both scanning the largest table in the schema. Six more FKs were unindexed on smaller tables; presentation_review.tag_id also CASCADEs. Dropped, on the other side: ix_image_record_sha256 was an exact duplicate of the index uq_image_record_sha256 already builds — two btrees on the same column of the highest-insert-rate table. The other six are single-column indexes a later composite superseded without the narrow one being retired; a btree on (a,b) already serves lookups on a. 0088 deliberately taught the models to declare BOTH sha256 indexes so they would describe reality. This changes the reality instead, and the models change with it — otherwise the next baseline.yml run reintroduces exactly the drift 0088 removed. CONCURRENTLY throughout, so building the image_tag index does not hold an ACCESS EXCLUSIVE lock over every write for the duration. The cost is that the migration cannot run in a transaction and so is not atomic: every statement is IF NOT EXISTS / IF EXISTS, making a re-run after a partial failure safe. The docstring carries the query for finding an INVALID index left by an interrupted CONCURRENTLY build. What the sweep found clean, for the record: all 43 tables have a primary key; all 51 FKs declare an explicit ON DELETE, so none silently blocks a delete; the three enum CHECKs match the code that writes them (rule 36). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017QHszn9H8VBvx5Ke8x1hvw --- alembic/versions/0089_index_hygiene.py | 120 ++++++++++++++++++++++ backend/app/models/backup_run.py | 11 +- backend/app/models/character_prototype.py | 4 +- backend/app/models/external_link.py | 6 +- backend/app/models/image_record.py | 8 +- backend/app/models/import_task.py | 2 + backend/app/models/presentation_review.py | 4 + backend/app/models/tag.py | 6 ++ backend/app/models/task_run.py | 9 +- 9 files changed, 159 insertions(+), 11 deletions(-) create mode 100644 alembic/versions/0089_index_hygiene.py diff --git a/alembic/versions/0089_index_hygiene.py b/alembic/versions/0089_index_hygiene.py new file mode 100644 index 0000000..0b22b5c --- /dev/null +++ b/alembic/versions/0089_index_hygiene.py @@ -0,0 +1,120 @@ +"""Index the seven unindexed FKs; drop the seven redundant indexes (#3300, #3301). + +Found by a structural sweep of the deployed schema done AFTER 0088 brought the +models and the migration chain into exact agreement. That agreement is what +0088 achieved, and it is worth being precise about what it does NOT prove: a +models-vs-chain diff shows the two describe the same schema. It says nothing +about whether that schema is right. Everything here was wrong in BOTH, which is +exactly the class of problem the reconciliation could not see. + +## Added: seven FK indexes + +`image_tag.tag_id` is the one that matters. The table's only index is +PRIMARY KEY (image_record_id, tag_id), which leads with the wrong column for +the two hottest things done with it: + + * the gallery's tag filter — services/tag_query.py builds + `image_tag.c.tag_id == tid` (and `.in_(tids)`) on every tag-scoped browse; + * ON DELETE CASCADE from `tag` — deleting or merging a tag makes Postgres + find that tag's rows before it can remove them. + +Both had to scan the largest table in the schema. The other six are the same +shape on much smaller tables; `presentation_review.tag_id` is the notable one, +since it also CASCADEs. + +## Dropped: seven redundant indexes + +`ix_image_record_sha256` was an exact duplicate. A UNIQUE constraint builds its +own index, so `uq_image_record_sha256` already covered the column and +`image_record` carried two btrees on `sha256` — on the highest-insert-rate +table in the system. + +The other six are single-column indexes that a later composite superseded +without the narrow one being retired. A btree on (a, b) already serves lookups +on `a`, so each was pure write amplification. `task_run` and `backup_run` are +append-heavy operational logs, which is where that cost lands hardest. + +Note for anyone reading 0088 next to this: 0088 deliberately taught the models +to declare BOTH sha256 indexes, so they would describe reality. That was right. +This migration changes the reality instead, and the models change with it. + +## CONCURRENTLY, and why this migration has no transaction + +`CREATE INDEX` takes an ACCESS EXCLUSIVE lock for the whole build, which on +`image_tag` means stalling every write for as long as it takes. CONCURRENTLY +builds without blocking writers, at the cost of two table passes and an +inability to run inside a transaction — hence `autocommit_block()`. + +The consequence to know about: this migration is NOT atomic. If it fails +partway, the work already done stays done. Every statement is therefore written +IF NOT EXISTS / IF EXISTS so that re-running it after a failure is safe rather +than an error. + +A failed CONCURRENTLY build also leaves an INVALID index behind — it is not +used by the planner and not repaired automatically. Find them with: + + SELECT c.relname FROM pg_index i + JOIN pg_class c ON c.oid = i.indexrelid + WHERE NOT i.indisvalid; + +Drop what that returns and re-run; nothing else is needed. + +Revision ID: 0089 +Revises: 0088 +Create Date: 2026-08-31 + +""" +from typing import Sequence, Union + +from alembic import op + +revision: str = "0089" +down_revision: Union[str, None] = "0088" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# (index name, table, column) — names match what the models render under +# base.py's naming convention, so autogenerate stays quiet after this. +MISSING_FK_INDEXES = ( + ("ix_image_tag_tag_id", "image_tag", "tag_id"), + ("ix_presentation_review_tag_id", "presentation_review", "tag_id"), + ("ix_presentation_review_conflict_tag_id", "presentation_review", "conflict_tag_id"), + ("ix_import_task_result_image_id", "import_task", "result_image_id"), + ("ix_external_link_attachment_id", "external_link", "attachment_id"), + ("ix_character_prototype_region_id", "character_prototype", "region_id"), + ("ix_backup_run_restored_from_id", "backup_run", "restored_from_id"), +) + +# (index name, table, column) — redundant; the second element of each pair in +# the docstring is what still covers the column after the drop. +REDUNDANT_INDEXES = ( + ("ix_image_record_sha256", "image_record", "sha256"), + ("ix_backup_run_kind", "backup_run", "kind"), + ("ix_backup_run_status", "backup_run", "status"), + ("ix_task_run_queue", "task_run", "queue"), + ("ix_task_run_status", "task_run", "status"), + ("ix_task_run_task_name", "task_run", "task_name"), + ("ix_external_link_post_id", "external_link", "post_id"), +) + + +def upgrade() -> None: + with op.get_context().autocommit_block(): + for name, table, column in MISSING_FK_INDEXES: + op.execute( + f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {name} " + f"ON {table} ({column})" + ) + for name, _table, _column in REDUNDANT_INDEXES: + op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {name}") + + +def downgrade() -> None: + with op.get_context().autocommit_block(): + for name, table, column in REDUNDANT_INDEXES: + op.execute( + f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {name} " + f"ON {table} ({column})" + ) + for name, _table, _column in MISSING_FK_INDEXES: + op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {name}") diff --git a/backend/app/models/backup_run.py b/backend/app/models/backup_run.py index 787e78c..d5e54cd 100644 --- a/backend/app/models/backup_run.py +++ b/backend/app/models/backup_run.py @@ -37,9 +37,12 @@ class BackupRun(Base): Index("ix_backup_run_tag_partial", "tag", postgresql_where=text("tag IS NOT NULL")), ) id: Mapped[int] = mapped_column(Integer, primary_key=True) - kind: Mapped[str] = mapped_column(String(16), nullable=False, index=True) + # No index=True: ix_backup_run_kind_started (above) already leads with + # `kind`, so a single-column index on it was pure write cost (#3301). + kind: Mapped[str] = mapped_column(String(16), nullable=False) status: Mapped[str] = mapped_column( - String(16), nullable=False, default="pending", index=True, + # No index=True — ix_backup_run_status_finished leads with `status`. + String(16), nullable=False, default="pending", server_default="pending", ) tag: Mapped[str | None] = mapped_column(String(64), nullable=True, index=True) @@ -57,7 +60,9 @@ class BackupRun(Base): manifest: Mapped[dict] = mapped_column( JSON, nullable=False, default=dict, server_default="{}", ) + # Self-referential FK, unindexed until 0089 (#3300): SET NULL has to find + # the rows pointing at a deleted run before it can null them. restored_from_id: Mapped[int | None] = mapped_column( ForeignKey("backup_run.id", ondelete="SET NULL"), - nullable=True, + nullable=True, index=True, ) diff --git a/backend/app/models/character_prototype.py b/backend/app/models/character_prototype.py index 191a29d..f281251 100644 --- a/backend/app/models/character_prototype.py +++ b/backend/app/models/character_prototype.py @@ -40,8 +40,10 @@ class CharacterPrototype(Base): ) # Provenance: the region this vector was copied from. SET NULL so pruning a # region doesn't delete the prototype mid-cycle (the next refresh reconciles). + # index=True added in 0089 — the FK was unindexed (#3300). region_id: Mapped[int | None] = mapped_column( - ForeignKey("image_region.id", ondelete="SET NULL"), nullable=True + ForeignKey("image_region.id", ondelete="SET NULL"), nullable=True, + index=True, ) diff --git a/backend/app/models/external_link.py b/backend/app/models/external_link.py index b06bf9b..4882125 100644 --- a/backend/app/models/external_link.py +++ b/backend/app/models/external_link.py @@ -57,11 +57,15 @@ class ExternalLink(Base): # — the same file linked twice in a post collapses to one row. Index("uq_external_link_post_url", "post_id", "url", unique=True), Index("ix_external_link_status", "status"), + # Unindexed FK (#3300). + Index("ix_external_link_attachment_id", "attachment_id"), ) id: Mapped[int] = mapped_column(Integer, primary_key=True) + # No index=True: uq_external_link_post_url (post_id, url) already leads + # with post_id (#3301). post_id: Mapped[int] = mapped_column( - ForeignKey("post.id", ondelete="CASCADE"), nullable=False, index=True + ForeignKey("post.id", ondelete="CASCADE"), nullable=False ) artist_id: Mapped[int | None] = mapped_column( ForeignKey("artist.id", ondelete="SET NULL"), nullable=True, index=True diff --git a/backend/app/models/image_record.py b/backend/app/models/image_record.py index 0fae950..f5f4050 100644 --- a/backend/app/models/image_record.py +++ b/backend/app/models/image_record.py @@ -59,9 +59,11 @@ class ImageRecord(Base): # On-disk identity path: Mapped[str] = mapped_column(Text, nullable=False, unique=True) - # index=True only: the UNIQUE half is the named constraint in - # __table_args__ above, matching what 0001 actually created. - sha256: Mapped[str] = mapped_column(String(64), nullable=False, index=True) + # Neither unique= nor index=: uq_image_record_sha256 in __table_args__ + # above creates its own index, and the separate ix_image_record_sha256 + # that 0001 also built was an exact duplicate of it — dropped in 0089 + # (#3301). Lookups by sha256 use the constraint's index. + sha256: Mapped[str] = mapped_column(String(64), nullable=False) phash: Mapped[str | None] = mapped_column(String(32), nullable=True, index=True) size_bytes: Mapped[int] = mapped_column(BigInteger, nullable=False) mime: Mapped[str] = mapped_column(String(64), nullable=False) diff --git a/backend/app/models/import_task.py b/backend/app/models/import_task.py index c3d9f11..e7921e6 100644 --- a/backend/app/models/import_task.py +++ b/backend/app/models/import_task.py @@ -31,6 +31,8 @@ class ImportTask(Base): __table_args__ = ( Index("ix_import_task_created_at_desc", text("created_at DESC")), + # Unindexed FK (#3300). + Index("ix_import_task_result_image_id", "result_image_id"), ) id: Mapped[int] = mapped_column(Integer, primary_key=True) batch_id: Mapped[int] = mapped_column( diff --git a/backend/app/models/presentation_review.py b/backend/app/models/presentation_review.py index e18e298..3e83b21 100644 --- a/backend/app/models/presentation_review.py +++ b/backend/app/models/presentation_review.py @@ -23,6 +23,10 @@ class PresentationReview(Base): __table_args__ = ( Index("ix_presentation_review_resolved_at", "resolved_at"), + # Both FKs to tag were unindexed (#3300); tag_id CASCADEs, so a tag + # delete had to scan this table to find its rows. + Index("ix_presentation_review_tag_id", "tag_id"), + Index("ix_presentation_review_conflict_tag_id", "conflict_tag_id"), ) image_record_id: Mapped[int] = mapped_column( ForeignKey("image_record.id", ondelete="CASCADE"), primary_key=True diff --git a/backend/app/models/tag.py b/backend/app/models/tag.py index cc23686..1b85936 100644 --- a/backend/app/models/tag.py +++ b/backend/app/models/tag.py @@ -71,6 +71,12 @@ image_tag = Table( Column("tag_id", ForeignKey("tag.id", ondelete="CASCADE"), primary_key=True), Column("source", String(32), nullable=False, default="manual", server_default="manual"), Column("created_at", DateTime(timezone=True), nullable=False, server_default=func.now()), + # The PK is (image_record_id, tag_id), which leads with the WRONG column + # for the two things that matter most here (#3300): the gallery's tag + # filter (tag_query.py builds `image_tag.c.tag_id == tid`) and the + # ON DELETE CASCADE from tag, which has to find a tag's rows to remove + # them. Without this index both scan the largest table in the schema. + Index("ix_image_tag_tag_id", "tag_id"), ) diff --git a/backend/app/models/task_run.py b/backend/app/models/task_run.py index c24ed46..2cd8b70 100644 --- a/backend/app/models/task_run.py +++ b/backend/app/models/task_run.py @@ -35,8 +35,10 @@ class TaskRun(Base): celery_task_id: Mapped[str] = mapped_column( String(64), nullable=False, index=True, ) - queue: Mapped[str] = mapped_column(String(32), nullable=False, index=True) - task_name: Mapped[str] = mapped_column(String(128), nullable=False, index=True) + # Neither carries index=True: ix_task_run_queue_started and + # ix_task_run_name_started already lead with these columns (#3301). + queue: Mapped[str] = mapped_column(String(32), nullable=False) + task_name: Mapped[str] = mapped_column(String(128), nullable=False) target_id: Mapped[int | None] = mapped_column(Integer, nullable=True) started_at: Mapped[datetime] = mapped_column( DateTime(timezone=True), nullable=False, index=True, @@ -46,7 +48,8 @@ class TaskRun(Base): ) duration_ms: Mapped[int | None] = mapped_column(Integer, nullable=True) status: Mapped[str] = mapped_column( - String(16), nullable=False, default="running", index=True, + # No index=True — ix_task_run_status_started leads with `status`. + String(16), nullable=False, default="running", server_default="running", ) error_type: Mapped[str | None] = mapped_column(String(128), nullable=True)