From 5c78aa332dd1180f254ebda7a536efa663c9d805 Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sun, 27 Sep 2026 12:53:23 -0600 Subject: [PATCH] collections: listing answers channel and download queries from covering indexes (#740) 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: