mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-25 18:40:06 -04:00
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 <noreply@anthropic.com>
This commit is contained in:
parent
20be5dfc27
commit
07980af10c
6 changed files with 101 additions and 13 deletions
|
|
@ -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')
|
||||
|
|
@ -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'),
|
||||
|
|
|
|||
|
|
@ -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':
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue