Skip to content

perf(host-core): replace session updated_at index with composite index - #1435

Merged
vastsa merged 3 commits into
mainfrom
worktree-fix+session-index
Oct 7, 2026
Merged

vastsa merged 3 commits into
mainfrom
worktree-fix+session-index

Conversation

@xiaobaZeo

Copy link
Copy Markdown
Collaborator

Fixes #1434

Motivation

When listing sessions, the ORDER BY s.updated_at DESC, s.id DESC clause could not fully utilize the existing idx_sessions_updated (which only covers updated_at DESC). This forces a TEMP B-TREE sort in SQLite memory.

Changes

  • Bumped SCHEMA_VERSION to 22
  • Added a database migration from v21 to v22 that replaces idx_sessions_updated with idx_sessions_updated_id(updated_at DESC, id DESC)
  • Updated schema.rs and database startup routing in repositories.rs
  • Added unit tests for the migration.

🤖 Generated with Claude Code

The idx_sessions_updated index only covered updated_at DESC. Because session queries sort by updated_at DESC, id DESC, SQLite was forced to load all matching sessions into memory and perform a TEMP B-TREE sort, significantly harming performance. This introduces a migration (v21 to v22) to drop the single-column index and replace it with a composite idx_sessions_updated_id.

Co-Authored-By: Claude Code <noreply@anthropic.com>

@muzimu217 muzimu217 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the perf claim against today's main (6881a2bfd) — the principle is sound and the composite index matches the real query shapes:

  • Hot session-list paths really do ORDER BY s.updated_at DESC, s.id DESC (crates/host-core/src/plugin_sessions.rs:861-865, session_collaboration/projections.rs:274) — exactly what idx_sessions_updated_id(updated_at DESC, id DESC) serves without a temp b-tree. This is where the reported latency comes from.
  • crates/host-core/src/sessions.rs:1291 orders by updated_at DESC alone — still served by the new index as a prefix, so dropping idx_sessions_updated is safe there.
  • crates/host-core/src/session_search.rs:114 uses updated_at DESC, id ASC — the composite can't serve that id ASC tiebreak, but the old single-column index didn't either; no regression.
  • Migration wiring is correct as far as I can read it: the v21→v22 step follows the v20→v21 pattern (backup + transaction + user_version bump), the legacy-path re-read of migrated_version after v20→v21 keeps chained upgrades on the loop path, and updating the v20_database_migrates_... fixture to pre-create the old index name is the right call now that fresh schema creates the composite directly.

Two things standing between this and mergeable, both visible in CI:

  1. cargo fmt --check is the only Rust failure (the failed step is "Check formatting"; tests/lint themselves aren't the blocker) — the new test has trailing-whitespace blank lines. cargo fmt fixes it.
  2. Base gate: needs a rebase onto latest main.

One suggestion, not a blocker: add an EXPLAIN QUERY PLAN regression asserting the session-list query uses idx_sessions_updated_id without USE TEMP B-TREE FOR ORDER BY. That pins the actual invariant #1434 is about (query plan), not just the DDL — a future query reshuffle that silently reintroduces the memory sort would otherwise pass this test suite.

@muzimu217

Copy link
Copy Markdown
Contributor

Update with local toolchain verification (rustup installed here, so we can now run the Rust gates directly):

  • cargo fmt --check reproduces exactly two spots: crates/host-core/src/db/migrations.rs:966 — the execute_batch string literal needs a trailing , after ...id DESC);" — and crates/host-core/src/db/tests.rs:1669-1683 — the execute_batch call needs rustfmt's method-chain indentation. cargo fmt -p host-core fixes both.
  • The migration logic itself is correct locally: migrates_v21_to_v22_replaces_session_index and the updated v20_database_migrates_the_session_checklist_with_a_backup fixture both pass on this head (cargo 1.99.0, stable).

So after cargo fmt + rebase onto latest main, everything we can check locally is green. The EXPLAIN QUERY PLAN regression suggestion from the review stands as a non-blocking follow-up.

vastsa added 2 commits October 7, 2026 15:44
The session index change must land against the current migration chain and project baseline.
The composite session index migration advances the database schema to version 22. Keep the fresh-database and legacy-upgrade assertions aligned so the host-core suite continues to cover current-schema startup.

Document the session-list index and keep the migration Rust-formatted for the landing gate.
@vastsa
vastsa merged commit 176a2e1 into main Oct 7, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance: SQLite TEMP B-TREE sort on session lists due to missing composite index

3 participants