From 07980af10cbe2f4db0e273d9b41d98933907e59b Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 27 Sep 2026 12:36:14 -0600 Subject: [PATCH] collections: listing answers channel and download queries from covering indexes The listing still read every channel row (for the summary columns) and every download row of the listed Collections (for the minimum frequency). Both tables store yt-dlp info_json, so rows are tens of KB and each sits on its own scattered page; selecting fewer columns did not help because the seek per row is the cost. From a cold disk cache that was 5 s per table, 10+ s total, on a large library. Two covering indexes, channel (collection_id, video_count, total_size) and download (collection_id, frequency), answer both queries without touching a row. The listing no longer loads Download objects at all; it computes min_download_frequency with one grouped query and drops the unused `downloads` array from listing rows (the detail endpoints keep it; the list views only read min_download_frequency). Adds a models-vs-migrations test (alembic check) so an index or column added to a model without a migration, or vice versa, fails the suite. Co-Authored-By: Claude Fable 5.1 --- ...6_09_28_1200_collection_listing_indexes.py | 29 +++++++++++++++ modules/videos/models.py | 2 ++ wrolpi/collections/lib.py | 33 ++++++++++------- wrolpi/collections/test/test_api.py | 12 ++++++- wrolpi/downloader.py | 2 ++ wrolpi/test/test_db_bootstrap.py | 36 +++++++++++++++++++ 6 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 alembic/versions/2026_09_28_1200_collection_listing_indexes.py diff --git a/alembic/versions/2026_09_28_1200_collection_listing_indexes.py b/alembic/versions/2026_09_28_1200_collection_listing_indexes.py new file mode 100644 index 00000000..d8ca602b --- /dev/null +++ b/alembic/versions/2026_09_28_1200_collection_listing_indexes.py @@ -0,0 +1,29 @@ +"""Covering indexes for the Collection listing. + +Listing Collections read every channel row (for video_count/total_size) and every download +row of those Collections (for the minimum frequency). Both tables store yt-dlp info_json, +so their rows are tens of KB and each sits on its own scattered page; from a cold disk cache +that was 10+ seconds. These indexes answer both queries without touching a row. + +Revision ID: 2026_09_28_1200 +Revises: 2026_09_27_1200 +""" +from alembic import op + +# revision identifiers, used by Alembic. +revision = '2026_09_28_1200' +down_revision = '2026_09_27_1200' +branch_labels = None +depends_on = None + + +def upgrade(): + op.execute('CREATE INDEX IF NOT EXISTS channel_collection_summary_idx' + ' ON channel (collection_id, video_count, total_size)') + op.execute('CREATE INDEX IF NOT EXISTS download_collection_frequency_idx' + ' ON download (collection_id, frequency)') + + +def downgrade(): + op.execute('DROP INDEX IF EXISTS channel_collection_summary_idx') + op.execute('DROP INDEX IF EXISTS download_collection_frequency_idx') diff --git a/modules/videos/models.py b/modules/videos/models.py index 750a4d5b..33b4012a 100644 --- a/modules/videos/models.py +++ b/modules/videos/models.py @@ -593,6 +593,8 @@ class Channel(ModelHelper, Base): __tablename__ = 'channel' __table_args__ = ( Index('channel_minimum_frequency_idx', 'minimum_frequency'), + # Covers the Collection listing's summary query; channel rows are wide (info_json). + Index('channel_collection_summary_idx', 'collection_id', 'video_count', 'total_size'), Index('channel_source_id', 'source_id'), Index('channel_total_size_idx', 'total_size'), Index('channel_video_count_idx', 'video_count'), diff --git a/wrolpi/collections/lib.py b/wrolpi/collections/lib.py index 03082b42..799ea8bd 100644 --- a/wrolpi/collections/lib.py +++ b/wrolpi/collections/lib.py @@ -8,7 +8,7 @@ import pathlib from typing import List, Optional, Dict from sqlalchemy import func -from sqlalchemy.orm import Session, selectinload +from sqlalchemy.orm import Session, selectinload, noload from wrolpi import flags from wrolpi.common import logger, get_relative_to_media_directory, TRACE_LEVEL, background_task @@ -73,9 +73,11 @@ def get_collections(session: Session, kind: Optional[str] = None) -> List[dict]: # Local imports to avoid circular import: collections -> videos -> collections from modules.videos.models import Channel - # Serializing a Collection reads its tag and downloads; load them in two batch queries rather - # than one lazy query per Collection (thousands of Collections on a large library). - query = session.query(Collection).options(selectinload(Collection.tag), selectinload(Collection.downloads)) + # Serializing a Collection reads its tag; load them in one batch query rather than one lazy + # query per Collection (thousands of Collections on a large library). Downloads are NOT + # loaded: download rows are wide (yt-dlp info_json) and the listing only needs the minimum + # frequency, which comes from a covering index below. + query = session.query(Collection).options(selectinload(Collection.tag), noload(Collection.downloads)) if kind: query = query.filter(Collection.kind == kind) @@ -85,6 +87,18 @@ def get_collections(session: Session, kind: Optional[str] = None) -> List[dict]: if not collections: return [] + # Batch query: minimum recurring download frequency per collection. Answered entirely by + # `download_collection_frequency_idx` (a covering index), so no download row is read. + from wrolpi.downloader import Download + collection_ids = [c.id for c in collections] + min_frequency_map = dict(session.query( + Download.collection_id, + func.min(Download.frequency), + ).filter( + Download.collection_id.in_(collection_ids), + Download.frequency > 0, + ).group_by(Download.collection_id).all()) + # Separate collections by kind for batch processing channel_ids = [c.id for c in collections if c.kind == 'channel'] generic_ids = [c.id for c in collections if c.kind not in ('domain', 'channel')] @@ -123,14 +137,9 @@ def get_collections(session: Session, kind: Optional[str] = None) -> List[dict]: result = [] for collection in collections: data = collection.__json__() - - # Compute minimum download frequency from all downloads - if collection.downloads: - # Filter to recurring downloads (frequency > 0) and get minimum - recurring_frequencies = [d.frequency for d in collection.downloads if d.frequency and d.frequency > 0] - data['min_download_frequency'] = min(recurring_frequencies) if recurring_frequencies else None - else: - data['min_download_frequency'] = None + # The listing does not carry each Collection's Downloads (see above); the detail endpoint does. + data.pop('downloads', None) + data['min_download_frequency'] = min_frequency_map.get(collection.id) # Add type-specific statistics from batch query results if collection.kind == 'domain': diff --git a/wrolpi/collections/test/test_api.py b/wrolpi/collections/test/test_api.py index 7554d566..2b5cb5e7 100644 --- a/wrolpi/collections/test/test_api.py +++ b/wrolpi/collections/test/test_api.py @@ -625,9 +625,19 @@ async def test_get_channel_collections_uses_summary_columns_and_batches_queries( assert data['total_size'] == channel.total_size > 0 assert data['min_download_frequency'] == DownloadFrequency.weekly assert data['tag_name'] == channel.tag_name - # BEGIN, collections, channel summaries, tags, downloads: fixed count regardless of channel count. + # The listing does not carry Downloads; the detail endpoint does. + assert 'downloads' not in data + # BEGIN, collections, min frequencies, channel summaries, tags: fixed count regardless of channel count. assert len(statements) <= 5, '\n'.join(statements) assert not any('file_group' in s for s in statements), 'Channel sizes must come from channel.total_size' + # channel and download rows are wide (info_json); the listing must be answered by covering indexes. + assert not any('info_json' in s for s in statements), 'No channel or download row may be loaded' + engine_plan = lambda sql: ' '.join(r[3] for r in test_session.execute(sa_text(f'EXPLAIN QUERY PLAN {sql}'))) # noqa + assert 'COVERING INDEX channel_collection_summary_idx' in engine_plan( + 'SELECT collection_id, id, video_count, total_size FROM channel WHERE collection_id IN (1, 2)') + assert 'COVERING INDEX download_collection_frequency_idx' in engine_plan( + 'SELECT collection_id, MIN(frequency) FROM download WHERE collection_id IN (1, 2) AND frequency > 0' + ' GROUP BY collection_id') @pytest.mark.asyncio diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 36937f17..c60109ef 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -245,6 +245,8 @@ class Download(ModelHelper, Base): # noqa __tablename__ = 'download' # noqa __table_args__ = ( Index('idx_download_collection_id', 'collection_id'), + # Covers the Collection listing's min-frequency query; download rows are wide (info_json). + Index('download_collection_frequency_idx', 'collection_id', 'frequency'), Index('idx_download_last_download_attempt', 'last_download_attempt'), ) id = Column(Integer, primary_key=True) diff --git a/wrolpi/test/test_db_bootstrap.py b/wrolpi/test/test_db_bootstrap.py index eac08b5b..681df056 100644 --- a/wrolpi/test/test_db_bootstrap.py +++ b/wrolpi/test/test_db_bootstrap.py @@ -211,6 +211,42 @@ def test_domain_collection_summary_migration_backfills(test_directory): assert conn.execute('SELECT item_count, total_size FROM collection WHERE id = 1').fetchone() == (1, 100) +def test_migrations_match_models(test_directory): + """`alembic check`: a database migrated to head matches the SQLAlchemy models (tables, columns, + indexes). Fails when a model gains an index or column without a migration, or vice versa.""" + from alembic.autogenerate import compare_metadata + from alembic.config import Config + from alembic import command + from alembic.migration import MigrationContext + from wrolpi.common import Base + from wrolpi.db import get_db_uri, create_wrolpi_engine + from wrolpi.vars import PROJECT_DIR + + (test_directory / 'config').mkdir(parents=True, exist_ok=True) + config = Config(str(PROJECT_DIR / 'alembic.ini')) + config.set_main_option('sqlalchemy.url', get_db_uri()) + command.upgrade(config, 'head') + + # Compare with alembic/env.py's own exclusions (FTS tables), plus any table a test module + # registered on the shared metadata (wrolpi/test/test_dates.py defines one). + env_source = (PROJECT_DIR / 'alembic' / 'env.py').read_text() + namespace = {} + exec(env_source[env_source.index('def include_object'):env_source.index('def process_revision_directives')], + namespace) + model_tables = {name for name in Base.metadata.tables if not name.startswith('test_')} + + def include_object(object_, name, type_, reflected, compare_to): + if type_ == 'table' and name not in model_tables and not reflected: + return False + return namespace['include_object'](object_, name, type_, reflected, compare_to) + + engine = create_wrolpi_engine(get_db_uri()) + with engine.connect() as conn: + context = MigrationContext.configure(conn, opts={'include_object': include_object, 'render_as_batch': True}) + diffs = compare_metadata(context, Base.metadata) + assert not diffs, diffs + + def test_bootstrap_lock(test_directory): """Only one process can hold the bootstrap lock.""" with db_bootstrap.bootstrap_lock() as acquired: