diff --git a/backend/alembic/versions/0095_videos_channel_cover_index.py b/backend/alembic/versions/0095_videos_channel_cover_index.py new file mode 100644 index 00000000..96ef400b --- /dev/null +++ b/backend/alembic/versions/0095_videos_channel_cover_index.py @@ -0,0 +1,47 @@ +"""videos: the per-channel index covers what the channel manager and the feed sidebar read (E31 S4d) + +The channel manager (`GET /api/channels`) sums each subscribed channel's videos: count, last upload, total +duration. The feed sidebar (`/feed/summary`, `/feed/years`) counts the reader's videos per channel and per +year under the default filters: available, not via explore, not a short, not live. Every one of them found +its rows through `ix_videos_channel_pub_desc` (channel_id, published_at DESC NULLS LAST, id) and then read +each video's heap page for the rest: ~270k rows at ~2.5 KB each, 681 MB on prod. That stays in cache only +while nothing else needs the memory. Measured on prod (2026-10-02): `/api/channels` 51–61 ms warm, but +2.25 s and 1.96 s on a first open in the browser and 6.4 s under EXPLAIN with 30k blocks read from disk; +`/feed/summary` and `/feed/years` ~140 ms warm, ~800 ms cold. + +The same key with those columns INCLUDEd makes them Index Only Scans. On a prod-copy localdev, after a +VACUUM, the manager's sums read 0 heap pages and the summary reads the 25 MB index instead of the heap. +The visibility map is what lets them skip the heap; prod's autovacuum kept it complete (relallvisible == +relpages). The key columns are unchanged, so every reader of the old index (the per-channel feed, the FK +cascade, the sync upsert, channel detail) rides this one. The old index is dropped. +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. +""" + +from alembic import op + +revision: str = "0095_videos_channel_cover_index" +down_revision: str | None = "0094_plex_genres_hu_everywhere" +branch_labels: str | None = None +depends_on: str | None = None + + +def upgrade() -> None: + with op.get_context().autocommit_block(): + op.execute( + "CREATE INDEX CONCURRENTLY IF NOT EXISTS 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)" + ) + 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)" + ) + op.execute("DROP INDEX CONCURRENTLY IF EXISTS ix_videos_channel_pub_cover") diff --git a/backend/app/models.py b/backend/app/models.py index 91b268c0..e7da5d8d 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -330,10 +330,12 @@ class Video(Base, TimestampMixin): __tablename__ = "videos" id: Mapped[str] = mapped_column(String(16), primary_key=True) # video id - # No standalone channel_id index: the composite `ix_videos_channel_pub_desc` - # (channel_id, published_at DESC NULLS LAST, id) from migration 0067 leads with channel_id, so it - # serves every channel_id lookup (FK cascade, sync upsert, channel-detail) AND the per-channel feed - # sort. Like the search_vector GIN, this composite is migration-defined, not an inline `index=True`. + # No standalone channel_id index: the composite `ix_videos_channel_pub_cover` + # (channel_id, published_at DESC NULLS LAST, id) from migration 0095 leads with channel_id, so it + # serves every channel_id lookup (FK cascade, sync upsert, channel-detail) and the per-channel feed + # sort. Its INCLUDEd columns make the channel manager's sums and the feed sidebar's counts Index Only + # Scans (no heap): a column those queries start reading belongs in it (test_channel_list_index_only). + # Like the search_vector GIN, this composite is migration-defined, not an inline `index=True`. channel_id: Mapped[str] = mapped_column(ForeignKey("channels.id", ondelete="CASCADE")) title: Mapped[str | None] = mapped_column(Text) # normalized for display (see app.titles) original_title: Mapped[str | None] = mapped_column(Text) # raw YouTube title, preserved diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index 9c9c3a52..3f63dfa9 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -6,7 +6,7 @@ from datetime import datetime, timezone from fastapi import APIRouter, BackgroundTasks, Depends from pydantic import BaseModel -from sqlalchemy import func, select, update +from sqlalchemy import Select, func, select, update from sqlalchemy.orm import Session from app.collation import collated, sorted_words @@ -40,6 +40,22 @@ router = APIRouter(prefix="/api/channels", tags=["channels"]) log = logs.get_logger(__name__) +def channel_aggregates(channel_ids: list[str]) -> Select: + """Per-channel stored count, last upload and total duration, in one grouped pass. Every column it + reads is in `ix_videos_channel_pub_cover` (migration 0095), so Postgres answers it from the index + alone; a test holds that (test_channel_list_index_only).""" + return ( + select( + Video.channel_id, + func.count(Video.id), + func.max(Video.published_at), + func.coalesce(func.sum(Video.duration_seconds), 0), + ) + .where(Video.channel_id.in_(channel_ids)) + .group_by(Video.channel_id) + ) + + @router.get("") def list_channels(user: User = Depends(current_user), db: Session = Depends(get_db)) -> list[dict]: rows = db.execute( @@ -49,24 +65,15 @@ def list_channels(user: User = Depends(current_user), db: Session = Depends(get_ .order_by(Subscription.priority.desc(), collated(Channel.title, lang_for_user(user))) ).all() - # Per-channel aggregates over the shared catalog in ONE grouped pass: stored count, last - # upload, total duration. Cheap — a single grouped scan, so computed on read rather than - # denormalised (measure-first: reads aren't frequent/hot enough to warrant cached columns). + # Per-channel aggregates over the shared catalog (channel_aggregates), computed on read rather than + # denormalised: an Index Only Scan over a ~20 MB index, where it used to read every video's heap page + # (681 MB on prod, a 2–6 s first open once something else had pushed it out of memory, E31 S4d). # (The Normal/Shorts/Live breakdown moved to the channel-detail page in R14, so it is no longer # computed here — the manager table dropped its N/S/L column.) channel_ids = [c.id for _, c in rows] agg: dict[str, dict] = {} if channel_ids: - for cid, n, last_up, total_dur in db.execute( - select( - Video.channel_id, - func.count(Video.id), - func.max(Video.published_at), - func.coalesce(func.sum(Video.duration_seconds), 0), - ) - .where(Video.channel_id.in_(channel_ids)) - .group_by(Video.channel_id) - ).all(): + for cid, n, last_up, total_dur in db.execute(channel_aggregates(channel_ids)).all(): agg[cid] = { "stored": n, "last_video_at": iso(last_up), diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index a1ddc43b..cef6ee07 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -10,6 +10,7 @@ stored in the DB and stay env-only). from dataclasses import dataclass +from sqlalchemy import select from sqlalchemy.orm import Session from app import logs, security @@ -307,7 +308,11 @@ def _raw_override(db: Session, key: str) -> str | None: per key so the operator gets a real signal without a per-request traceback. Recovery is documented in docs/self-hosting.md: the key must never change once secrets are stored. (B-core-2) """ - row = db.get(SystemConfig, key) + return _raw_of(db.get(SystemConfig, key), key) + + +def _raw_of(row: SystemConfig | None, key: str) -> str | None: + """`_raw_override` over a row already read (describe() reads them all in one query).""" if row is None or row.value is None: return None if not row.encrypted: @@ -327,8 +332,11 @@ def _raw_override(db: Session, key: str) -> str | None: def get(db: Session, key: str): """Effective value: the DB override (typed) if set, else the env/config default.""" - s = _BY_KEY[key] - raw = _raw_override(db, key) + return _effective(_BY_KEY[key], _raw_override(db, key)) + + +def _effective(s: ConfigSpec, raw: str | None): + """`get` over an override already read.""" if raw is None: return _default(s) if raw == "" and not s.allow_empty: @@ -420,8 +428,12 @@ def describe(db: Session) -> dict: """Admin-UI metadata + current values, grouped. Secret values are never echoed — only whether an override is set and whether an env default exists.""" groups: dict[str, list[dict]] = {} + # Every override in one query: a db.get per key was one round trip each (two for a key with no + # row, which the session cannot cache), ~40 ms of the page's ~55 on prod (E31 S4d). + rows = {r.key: r for r in db.scalars(select(SystemConfig))} for s in SPECS: - is_set = db.get(SystemConfig, s.key) is not None + row = rows.get(s.key) + is_set = row is not None item: dict = { "key": s.key, "type": s.type, @@ -436,7 +448,7 @@ def describe(db: Session) -> dict: if s.secret: item["default_is_set"] = bool(_default(s)) else: - item["value"] = get(db, s.key) + item["value"] = _effective(s, _raw_of(row, s.key)) item["default"] = _default(s) groups.setdefault(s.group, []).append(item) sections = [ diff --git a/backend/tests/test_channel_list_index_only.py b/backend/tests/test_channel_list_index_only.py new file mode 100644 index 00000000..cdcbd72b --- /dev/null +++ b/backend/tests/test_channel_list_index_only.py @@ -0,0 +1,83 @@ +"""The channel manager's sums and the feed sidebar's counts read the index alone (E31 S4d). + +`GET /api/channels` counts each subscribed channel's videos and sums their durations; `/feed/summary` +and `/feed/years` count the reader's videos per channel and per year under the default filters. They +used to read every video's heap page for that (681 MB on prod), a 2–6 s first open once something else +had pushed the table out of memory. Migration 0095 makes `ix_videos_channel_pub_cover` carry every +column those queries read, so Postgres answers them with Index Only Scans. This test holds the queries +and the index together: a column added to one and not the other fails it. DB lane. +""" + +import re + +from sqlalchemy import text +from sqlalchemy.dialects import postgresql + +from app.feedquery import filtered_query +from app.models import Video +from app.routes.channels import channel_aggregates + +INDEX = "ix_videos_channel_pub_cover" + +# The feed's own defaults (routes/feed.py _feed_params): what the sidebar asks on a plain open. +FEED_DEFAULTS = { + "tags": [], + "tag_modes": {"language": "or", "topic": "or", "other": "or"}, + "untagged": False, + "channel_ids": [], + "q": None, + "min_duration": None, + "max_duration": None, + "max_age_days": None, + "published_after": None, + "published_before": None, + "show_normal": True, + "include_shorts": False, + "include_live": False, + "show": "unwatched", + "scope": "my", + "library_source": "organic", +} + + +def _index_columns(db) -> list[str]: + # Key columns first, then the INCLUDE ones, in the index's own order. + return list( + db.execute( + text( + "SELECT a.attname FROM pg_index i " + "JOIN pg_class c ON c.oid = i.indexrelid " + "CROSS JOIN LATERAL unnest(i.indkey) WITH ORDINALITY AS k(attnum, n) " + "JOIN pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = k.attnum " + "WHERE c.relname = :name ORDER BY k.n" + ), + {"name": INDEX}, + ).scalars() + ) + + +def _video_columns(stmt) -> set[str]: + # The columns of `videos` the statement as SENT reads: the compiled SQL, not the expression tree, + # which still carries the projection a with_only_columns replaced. + sql = str(stmt.compile(dialect=postgresql.dialect())) + return set(re.findall(r"\bvideos\.(\w+)", sql)) + + +def _assert_covered(db, read: set[str]) -> None: + held = _index_columns(db) + assert held, f"{INDEX} is missing" + # It leads with channel_id, so it also serves every per-channel lookup the old index did. + assert held[0] == "channel_id" + assert read <= set(held), f"read but not in {INDEX}: {sorted(read - set(held))}" + + +def test_the_channel_sums_read_only_columns_the_index_holds(db): + _assert_covered(db, _video_columns(channel_aggregates(["UC_x"]))) + + +def test_the_feed_sidebar_counts_read_only_columns_the_index_holds(db, make_user): + # feed_summary groups the default filter by channel, feed_years by publish date. + base, _rank = filtered_query(db, make_user(), **FEED_DEFAULTS) + _assert_covered( + db, _video_columns(base.with_only_columns(Video.channel_id, Video.published_at)) + ) diff --git a/backend/tests/test_sysconfig_describe_queries.py b/backend/tests/test_sysconfig_describe_queries.py new file mode 100644 index 00000000..9c1aff47 --- /dev/null +++ b/backend/tests/test_sysconfig_describe_queries.py @@ -0,0 +1,39 @@ +"""The admin Configuration page reads every override in one query (E31 S4d). + +`sysconfig.describe()` did a `db.get` per key to tell whether an override was set, and `get()` did one +more for its value: ~55 keys, and two round trips for a key with no row, since the session cannot cache +an absence. That was ~40 ms of the page's ~55 on prod. It now reads the table once. DB lane. +""" + +from sqlalchemy import event + +from app import sysconfig + + +def _statements(db, fn) -> int: + seen: list[str] = [] + engine = db.get_bind() + listener = lambda *a, **k: seen.append(a[2]) # noqa: E731 — (conn, cursor, statement, …) + event.listen(engine, "before_cursor_execute", listener) + try: + fn() + finally: + event.remove(engine, "before_cursor_execute", listener) + return len(seen) + + +def test_describe_reads_the_overrides_once(db): + sysconfig.set_value(db, "quota_daily_budget", 5000) + db.commit() + assert _statements(db, lambda: sysconfig.describe(db)) == 1 + + +def test_describe_shows_the_stored_value_and_the_default(db): + sysconfig.set_value(db, "quota_daily_budget", 5000) + db.commit() + items = {i["key"]: i for g in sysconfig.describe(db)["groups"].values() for i in g} + assert items["quota_daily_budget"]["is_set"] is True + assert items["quota_daily_budget"]["value"] == 5000 + other = next(i for k, i in items.items() if k != "quota_daily_budget" and not i["secret"]) + assert other["is_set"] is False + assert other["value"] == other["default"] diff --git a/backend/tests/test_sysconfig_registry.py b/backend/tests/test_sysconfig_registry.py index d933ec30..4190f71e 100644 --- a/backend/tests/test_sysconfig_registry.py +++ b/backend/tests/test_sysconfig_registry.py @@ -67,19 +67,23 @@ def test_the_env_accepts_any_letter_case_and_rejects_an_unknown_value(): class _Row: - def __init__(self, value): - self.value, self.encrypted = value, False + def __init__(self, key, value): + self.key, self.value, self.encrypted = key, value, False class _FakeDb: """Just enough of a Session for sysconfig's reads: one stored override row per key.""" def __init__(self, **rows): - self.rows = {k: _Row(v) for k, v in rows.items()} + self.rows = {k: _Row(k, v) for k, v in rows.items()} def get(self, _model, key): return self.rows.get(key) + def scalars(self, _stmt): + # describe() reads every override at once (E31 S4d). + return list(self.rows.values()) + def test_a_stored_value_reads_back_canonical_and_a_stale_one_falls_back_to_the_default(): assert sysconfig.get_str(_FakeDb(plex_transcode_hw="OFF"), "plex_transcode_hw") == "off"