diff --git a/backend/alembic/versions/0095_videos_channel_cover_index.py b/backend/alembic/versions/0095_videos_channel_cover_index.py index 96ef400b..d6b0944e 100644 --- a/backend/alembic/versions/0095_videos_channel_cover_index.py +++ b/backend/alembic/versions/0095_videos_channel_cover_index.py @@ -17,10 +17,13 @@ cascade, the sync upsert, channel detail) rides this one. The old index is dropp tests/test_channel_list_index_only.py holds the queries and this column list together. CONCURRENTLY (autocommit_block) builds it without blocking a live prod. IF (NOT) EXISTS keeps the drift -test's downgrade → re-upgrade round trip idempotent. +test's downgrade → re-upgrade round trip idempotent. A re-run after a build that failed part-way rebuilds +the INVALID leftover, and the old index is dropped only once the new one is valid: a re-run that kept the +leftover and dropped the old one left `videos` with no usable channel_id index (E31 S5 review). """ from alembic import op +from sqlalchemy import text revision: str = "0095_videos_channel_cover_index" down_revision: str | None = "0094_plex_genres_hu_everywhere" @@ -28,20 +31,49 @@ branch_labels: str | None = None depends_on: str | None = None +def _valid(name: str) -> bool | None: + """The index's `indisvalid`, or None when there is no index of that name.""" + row = op.get_bind().execute( + text( + "SELECT i.indisvalid FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid " + "WHERE c.relname = :name" + ), + {"name": name}, + ).first() + return None if row is None else bool(row[0]) + + +def _build(name: str, ddl: str) -> None: + """CREATE INDEX CONCURRENTLY `name` (`ddl` is what follows the name), re-runnable. A build that + failed part-way leaves an INVALID index under the name, and `IF NOT EXISTS` would keep it: the + planner never uses it, and nothing says so (E31 S5 review). Such a leftover is dropped and built + again.""" + if _valid(name) is False: + op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {name}") + op.execute(f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {name} {ddl}") + + +def _swap(build: str, ddl: str, drop: str) -> None: + _build(build, ddl) + if not _valid(build): + raise RuntimeError(f"{build} did not build valid; {drop} is kept") + op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {drop}") + + def upgrade() -> None: with op.get_context().autocommit_block(): - op.execute( - "CREATE INDEX CONCURRENTLY IF NOT EXISTS ix_videos_channel_pub_cover " + _swap( + "ix_videos_channel_pub_cover", "ON videos (channel_id, published_at DESC NULLS LAST, id) " - "INCLUDE (duration_seconds, unavailable_since, via_explore, is_short, live_status)" + "INCLUDE (duration_seconds, unavailable_since, via_explore, is_short, live_status)", + "ix_videos_channel_pub_desc", ) - op.execute("DROP INDEX CONCURRENTLY IF EXISTS ix_videos_channel_pub_desc") def downgrade() -> None: with op.get_context().autocommit_block(): - op.execute( - "CREATE INDEX CONCURRENTLY IF NOT EXISTS ix_videos_channel_pub_desc " - "ON videos (channel_id, published_at DESC NULLS LAST, id ASC)" + _swap( + "ix_videos_channel_pub_desc", + "ON videos (channel_id, published_at DESC NULLS LAST, id ASC)", + "ix_videos_channel_pub_cover", ) - op.execute("DROP INDEX CONCURRENTLY IF EXISTS ix_videos_channel_pub_cover") diff --git a/backend/alembic/versions/0096_videos_sort_indexes.py b/backend/alembic/versions/0096_videos_sort_indexes.py index 5612d2a6..2f20b466 100644 --- a/backend/alembic/versions/0096_videos_sort_indexes.py +++ b/backend/alembic/versions/0096_videos_sort_indexes.py @@ -28,6 +28,7 @@ test's downgrade → re-upgrade round trip idempotent. """ from alembic import op +from sqlalchemy import text revision: str = "0096_videos_sort_indexes" down_revision: str | None = "0095_videos_channel_cover_index" @@ -47,10 +48,32 @@ INDEXES = { } +def _valid(name: str) -> bool | None: + """The index's `indisvalid`, or None when there is no index of that name.""" + row = op.get_bind().execute( + text( + "SELECT i.indisvalid FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid " + "WHERE c.relname = :name" + ), + {"name": name}, + ).first() + return None if row is None else bool(row[0]) + + +def _build(name: str, ddl: str) -> None: + """CREATE INDEX CONCURRENTLY `name` (`ddl` is what follows the name), re-runnable. A build that + failed part-way leaves an INVALID index under the name, and `IF NOT EXISTS` would keep it: the + planner never uses it, and nothing says so (E31 S5 review). Such a leftover is dropped and built + again.""" + if _valid(name) is False: + op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {name}") + op.execute(f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {name} {ddl}") + + def upgrade() -> None: with op.get_context().autocommit_block(): for name, columns in INDEXES.items(): - op.execute(f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {name} ON videos ({columns})") + _build(name, f"ON videos ({columns})") def downgrade() -> None: diff --git a/backend/tests/test_migration_rerun_invalid_index.py b/backend/tests/test_migration_rerun_invalid_index.py new file mode 100644 index 00000000..6d419755 --- /dev/null +++ b/backend/tests/test_migration_rerun_invalid_index.py @@ -0,0 +1,53 @@ +"""A concurrent index build that failed part-way, then re-run (E31 S5 review), DB-lane. + +CREATE INDEX CONCURRENTLY that fails leaves an INVALID index under its name, and alembic_version is not +stamped, so the next start runs the migration again. `IF NOT EXISTS` matched the name and kept the +leftover, which the planner never uses; 0095 then dropped the old channel_id index, leaving `videos` +with no usable one. A re-run now rebuilds the leftover and drops the old index only once the new one is +valid. +""" + +from alembic import command +from alembic.config import Config +from sqlalchemy import text + +from app.db import engine + + +def _index(name: str) -> tuple[bool, str] | None: + with engine.connect() as conn: + row = conn.execute( + text( + "SELECT i.indisvalid, pg_get_indexdef(i.indexrelid) FROM pg_index i " + "JOIN pg_class c ON c.oid = i.indexrelid WHERE c.relname = :name" + ), + {"name": name}, + ).first() + return None if row is None else (bool(row[0]), row[1]) + + +def _leave_invalid(name: str) -> None: + """What a build that failed part-way leaves behind: the name taken, the index INVALID.""" + with engine.connect().execution_options(isolation_level="AUTOCOMMIT") as conn: + conn.execute(text(f"CREATE INDEX {name} ON videos (id)")) + conn.execute( + text(f"UPDATE pg_index SET indisvalid = false WHERE indexrelid = '{name}'::regclass") + ) + + +def test_a_rerun_rebuilds_an_invalid_leftover_before_dropping_the_old_index(db): + db.rollback() # CONCURRENTLY waits for every open transaction, this session's too + cfg = Config("alembic.ini") + command.downgrade(cfg, "0094_plex_genres_hu_everywhere") + try: + _leave_invalid("ix_videos_channel_pub_cover") + _leave_invalid("ix_videos_views_desc") + finally: + command.upgrade(cfg, "head") + + cover = _index("ix_videos_channel_pub_cover") + assert cover is not None and cover[0], cover + assert "INCLUDE (duration_seconds" in cover[1] + assert _index("ix_videos_channel_pub_desc") is None + views = _index("ix_videos_views_desc") + assert views is not None and views[0] and "view_count DESC NULLS LAST" in views[1], views