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)