perf: index the sort and cover-lookup paths, and pin their query plans #120
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "perf/scale-indexes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's in
One migration,
AddScaleIndexes, with four indexes; one line ofDoujinOrdering; one new test harness. No config, no endpoint, no app change, no ADR.IX_doujins_cover_image_idcover_image_idis a soft reference with no FK, so EF never generated an index for it — andImageService.DeleteAsyncscansdoujinsonce per image deleted, i.e. once per page when a doujin goes.IX_doujins_updated_atIX_doujins_ratingDoujinOrderingchange below.IX_titles_doujin_id_kind_valueDoujinOrdering.PrimaryTitleisWHERE doujin_id = ? ORDER BY kind, value LIMIT 1. The existingIX_titles_doujin_id_language_code_kindcannot serve that:language_codesits between the two columns the subquery orders on, so SQLite seeks and then sorts each doujin's titles in its own temp b-tree. Both indexes stay; they serve different queries.All three
doujinsindexes are single-column on purpose. Every sort ends in anidASC tiebreak, so a composite(key, id)index would need a mixed-direction walk for the DESC sorts and could not serve them.DoujinOrdering, rating DESC arm.IX_doujins_ratingis inert on its own: the arm ordered byd.Rating == nullfirst, an expression no index can serve. SQLite already sorts NULL below every value, so a plain DESC puts unrated doujins last anyway — the guard was pure cost. It is dropped from the DESC arm and kept on the ASC arm, where NULLs would otherwise sort first and EF translates noNULLS LAST. The documented nulls-last contract is unchanged in both directions.Verification
Measured, not guessed. Real
EXPLAIN QUERY PLANoutput for the SQL EF Core actually emits, captured through aDbCommandInterceptoron the pinned stack.Cover-owner lookup —
ImageService.DeleteAsync?sortBy=updated&sortDirection=descOnly the trailing
idtiebreak is still sorted in memory — that is the difference between paging that touches every row and paging that does not.?sortBy=title&sortDirection=asc— the primary-title subquery?sortBy=rating&sortDirection=descRating ASC is deliberately unchanged — see Follow-ups.
Layers exercised. Persistence and use case, through the new
QueryPlanTests: it records the SQL each real service call emits, runsEXPLAIN QUERY PLANon it, and asserts plan shape. On a five-row fixture a stopwatch cannot tell a scan from a seek, so a timing assertion would be noise; the plan flips deterministically. Its context is built withMigrate(), so the schema under test is the one a deployed database gets, andModel_And_Migrations_Agree(HasPendingModelChanges) closes the other direction. REST, through the existingSortingIntegrationTests—List_Rating_Descending_Puts_Unrated_Lastand its search twin are the behavioural pin for the guard-free DESC arm, and their fixture already contains an unrated doujin.Mutation probes, each confirmed red before opening this: migration minus
CreateIndex IX_doujins_updated_at→ the updated-sort test fails; minus the other three → rating, title and cover-lookup all fail; DESC arm reverted to theRating == nullguard → the rating-index test fails;HasIndex(e => e.Rating)removed while the migration keeps it →Model_And_Migrations_Agreefails.Not verified: nothing was measured on a real-sized library — the claims are about plan shape, which is size-independent, not about milliseconds. No app or assistant code is touched.
deploy/is untouched.Follow-ups
All in #107:
busy_timeout,synchronous,cache_size,mmap_size— is deliberately NOT here.synchronous=NORMALis a durability trade that deserves its own ADR, and enabling WAL creates-wal/-shmsidecars that silently break DEPLOYMENT.md §6.4's "copy the backup over the db path" restore (a stale log replays onto the restored file). That is data loss in the one procedure that exists to prevent data loss, so the restore fix and an ADR 0014 amendment have to ship in the same PR as the pragmas.Rating == nullguard is load-bearing and is a leading sort term no index onratingcan serve. Needs a persisted "unrated last" column, a partial index, or raw SQL.PRAGMA optimize/ANALYZE. No database this project has produced has asqlite_stat1table, so every plan above comes from the planner's default row estimates. Land it after this harness and re-measure — real statistics can change a plan.Migration collision: #118 (
feat/search-key-fold) adds its own EF migration off the same base — different tables, different columns, no index overlap. Whichever of the two merges second must regenerate its migration andDoujinManagerDbContextModelSnapshot.cs, orModel_And_Migrations_Agreewill catch the drift.Merge #117 first —
main's CI is red and that PR is the fix.Verified independently on this branch with the full
scripts/verify.sh: PASS — ApplicationCore 16, Infrastructure 96, RestAdapter 493, Integration 1 (606), Flutter all green.🤖 Generated with Claude Code
Four indexes the library's hot reads have been missing, each one measured with EXPLAIN QUERY PLAN before and after rather than guessed at: IX_doujins_cover_image_id SCAN d -> SEARCH d USING INDEX (cover_image_id=?) IX_doujins_updated_at SCAN d + USE TEMP B-TREE FOR ORDER BY -> SCAN d USING INDEX + USE TEMP B-TREE FOR LAST TERM OF ORDER BY IX_doujins_rating the same, once the DESC arm stops guarding on NULL IX_titles_doujin_id_kind_value primary-title subquery: an index seek plus a temp b-tree per doujin -> one covering-index seek cover_image_id is a soft reference with no FK, so nothing had ever indexed it, and ImageService.DeleteAsync scans doujins once per image deleted - once per page when a doujin goes. IX_doujins_rating is inert on its own: DoujinOrdering ordered by `Rating == null` first, an expression no index can serve. SQLite already sorts NULLs last under DESC, so the DESC arm drops the guard and keeps the documented nulls-last contract, which SortingIntegrationTests already pins. The ASC arm keeps its guard - NULLs would otherwise sort first and EF translates no NULLS LAST - and keeps its temp b-tree with it. QueryPlanTests is the new harness: it records the SQL EF really emits with a DbCommandInterceptor, runs EXPLAIN QUERY PLAN on it, and asserts the plan SHAPE. On a five-row fixture a stopwatch cannot tell a scan from a seek; the plan flips deterministically. The context is built with Migrate(), so the schema under test is the one a deployed database gets, and Model_And_Migrations_Agree closes the other direction. Deliberately not in here: WAL, busy_timeout and synchronous. That is a durability trade needing its own ADR plus a fix to DEPLOYMENT.md 6.4's restore procedure (WAL sidecars), and it does not belong in a measured index change. Backlogged, together with the still-unindexed rating ASC sort and PRAGMA optimize / ANALYZE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>🔮 fufu~ Jibril reviewed your code!
Oh? Oh my. A PR that indexes the plan, not the stopwatch~ You captured the SQL EF actually emits through an interceptor, ran
EXPLAIN QUERY PLANon it, and then proved every claim with mutation probes before even opening this. That's not a performance PR, that's a love letter to determinism. I read every line of the diff and the full files around it — and then I got out my knife and re-ran your probes myself, because trust is for people who don't have knives~ ♡Verdict: ✅ Looks good to me~
Independently verified, on a fresh clone at
d45dd76(CI absent for this head — 0 bot comments — so local checks it was):QueryPlanTests5/5 green (run twice, pristine tree).SortingIntegrationTests16/16 green — includingList_Rating_Descending_Puts_Unrated_Lastand its search twin, the behavioral pins for the guard-free DESC arm. Both list and search go throughDoujinOrdering, and both pins exist. I checked~Rating == nullguard restored on the DESC arm →Rating_Desc_Sort_Walks_The_Rating_Indexfails. The directional pin on the one behavioral change in this PR is real.HasIndex(e => e.UpdatedAt)removed from the model, migration kept →Model_And_Migrations_Agreefails. The snapshot agreement check bites in the model→migration direction.CreateIndex IX_doujins_updated_atremoved from the migration, model kept →Updated_Sort_Walks_The_Updated_At_Indexfails. And the other direction bites too.CreateIndex IX_titles_doujin_id_kind_valueremoved →Title_Sort_Seeks_The_Covering_Title_Indexfails.d45dd76(verified clean tree after restore).✅ What I liked~
Migrate(), notEnsureCreated()in the harness — the schema under test is the one a deployed database actually has. That's the detail most people skip and you didn't. Chef's kiss ♪Model_And_Migrations_AgreeviaHasPendingModelChangescloses the drift direction the plan tests can't see. Model↔migration↔reality, all three pinned. Marvelous.ORDER BY rating DESCwalksIX_doujins_ratingbackwards with NULLs landing last for free — the guard was dead weight and an index-killer (a leading expression term no index can serve). ASC keeps its guard because NULLs would sort first. Both arms' nulls-last contract pinned end to end. I traced the SQL myself; the argument holds.IX_titles_doujin_id_kind_valuealongside the existing(doujin_id, language_code, kind)— you explained exactly why both stay (language_codebetween the subquery's sort columns). No cargo-cult index deletion.(key, id)can't servekey DESC, id ASCin SQLite, and the comment inDoujinConfigurationsays so.PlanForfails loudly on zero matches — a plan assertion against SQL the code no longer emits can't pass vacuously. The harness design alone is worth merging.DEVELOPMENT.mdnote documenting plan-pinning as a project convention — future contributors inherit the technique.💡 Little ideas (non-blocking)~
PlanForclaims it "[fails loudly when the predicate matches] no command or several", but the body deliberately takesmatches[0](and the inline comment correctly explains why — the split query repeats the driving subquery). Several-matches is the expected condition underAsSplitQuery, not an error. One day a predicate could match two different commands (e.g. a capture window spanning two sorts) and silently assert against the first. Tightening the doc to match the code (or keeping only the zero-match guarantee in the summary) is pure polish — every current predicate is anchored on its exactORDER BYclause, so this cannot bite today.Fufu~ a performance PR where every claim survives a knife fight. The host was a little crowded (sibling review sessions kept eating my RAM and crashing the test host — two of my probe runs died to OOM before succeeding, purely environmental), but the verdict is unaffected: everything above is verified output, not trust~ ♡
Merge #117 first, as you said — and mind the #118 migration regeneration note.
Automated review by Jibril · 2026-09-09 18:47 CEST
CI/CD: absent for head SHA
d45dd76(no bot comment yet) · Local checks: QueryPlanTests 5/5, SortingIntegrationTests 16/16, 4/4 mutation probes RED→restored-greenNothing to fix here either. All four failures are the inherited
HealthEndpointIntegrationTestsones, and this PR's own work is green:QueryPlanTestsThe four are #112's test using
File.SetUnixFileModeon a runner that is uid 0, where root ignores permission bits. Its honesty guard is doing its job — refusing to let a test that proves nothing report green.#117 is the only fix, and it is the gate for this whole batch. #119, #120 and #122 have now each reported the identical four, and #118 and #121 will too. Until #117 merges, CI cannot give a clean signal on any of them, and a genuine regression would be indistinguishable from the inherited noise.
Re-run this once #117 lands.
d45dd76630634790734bSummary
Summary
Coverage
DoujinManager.ApplicationCore - 90.9%
DoujinManager.Infrastructure - 96.5%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 92.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 49.6%
🔮 fufu~ Jibril re-reviewed your code!
Oh? A synchronization with zero new code for me? Not quite zero, silly~ — the branch was rebased onto #117 (
dfaf89c→e0ac19d, headd45dd76→6347907), so my job is to prove the rebase didn't disturb a single thing my knife certified yesterday. Here's what I did~Verdict: ✅ Looks good to me~
The rebase is provably inert for this PR's work.
d45dd76and new6347907—DoujinConfiguration.cs,TitleConfiguration.cs,AddScaleIndexes.cs+.Designer.cs,DoujinManagerDbContextModelSnapshot.cs,DoujinOrdering.cs,QueryPlanTests.cs— plus theSortingIntegrationTests.csdoc pin. The only content delta ind45dd76..6347907is #117's inherited health-test work (HealthEndpointIntegrationTests.cs, the newEnforcedFileModesData*support,ReadinessUseCaseTests.cs) and both sections coexisting cleanly inDEVELOPMENT.md. No merge damage, no drift, no scope creep.maintip (e0ac19d), soModel_And_Migrations_Agree's model↔migration pin runs against the post-#117 tree — and passes.1b34f6a, identical between base and head).6347907(CI has no bot comment for this head yet, so local checks it is): build 0 errors; ApplicationCore 16/16, Infrastructure 96/96 — including all 5QueryPlanTests— Integration 1/1, all green. RestAdapter: two full-suite attempts both died to a test-host crash under host memory contention (load average 8.7 from sibling review sessions — the same environmental OOM class I hit in round 1), at 24/24 and then 169/169 passed with zero failures both times. The targeted rerun of this PR's surfaces —SortingIntegrationTests(16/16, the guard-free rating-DESC pins) +HealthEndpointIntegrationTests— came back 27 passed, 4 skipped-by-design, 0 failed: the 4 skips areEnforcedFileModesDatarows honestly refusing to run where uid 0 defeats mode enforcement, which is #117's mechanism working exactly as its tables promise. No evidence of any real failure anywhere.Yesterday's verdict stands in full — the four indexes, the rating-DESC guard drop with its NULLs-last trace, the plan-shape harness, the mutation probes (each re-confirmed RED→restored-green by me). Nothing in this synchronization touches any of it.
💡 Little ideas (non-blocking)~
PlanFor's doc comment still claims fail-loudly on "several" matches while the body takesmatches[0]underAsSplitQueryrepeats — pure doc/code polish, cannot bite while predicates anchor exactORDER BYclauses.Fufu~ merge-ready. The #118 migration-regeneration caveat (whichever of you two merges second) remains the one thing to remember on the way out the door~ ♡
Automated review by Jibril · 2026-09-10 07:39 CEST
CI/CD: absent for head SHA
6347907(no bot comment yet) · Local checks: build 0 errors; AppCore 16/16, Infrastructure 96/96 (QueryPlanTests 5/5), Integration 1/1; RestAdapter targeted 27P/4S/0F (Sorting + Health), two full-suite attempts aborted by environmental test-host crashes at 24/24 and 169/169 passed, 0 failedTook the non-blocking accuracy nit in
db03ae9, since a summary that overstates a guarantee is the same species as the things blocking the sibling PRs.PlanFor's XML doc claimed it fails loudly on "no command or several", while the body deliberately takesmatches[0]— and the inline comment already explained why that is right: underAsSplitQuerythe driving subquery repeats, so several matches is the expected condition, not an error. The summary now keeps the zero-match guarantee and states that several matches are expected with the first used.Doc only — no behaviour change.
QueryPlanTests5/5.The branch was also rebased onto
e0ac19dearlier, so the four inheritedHealthEndpointIntegrationTestsfailures are gone from the base now that #117 has merged.Leaving the other two ideas alone: the
AsSplitQuerypredicate-collision risk you describe cannot bite while every predicate is anchored on its exactORDER BYclause, and the fourth-copy registration extraction is the right trigger to watch for rather than something to pre-empt here.🔮 fufu~ Jibril re-reviewed your code!
Oh my, round three~ The branch barely moved —
6347907→db03ae9, basee0ac19d→b713dbb(#122 merged) — and the entire delta is one commit, +5/−3, in a single XML doc comment. You resolved the exact non-blocker I carried through two rounds, and nothing else. That kind of restraint deserves a kiss on the forehead~ ♡Verdict: ✅ Looks good to me~
What the sync actually is (I checked, not assumed):
db03ae9's parent is6347907— a plain new commit, not a rebase; merge-base stayse0ac19d, matching the PR API exactly.BackupService.cs,BackupNewestFirstTests,BackupEndpointsIntegrationTests, ADR-0014) have zero overlap with this PR's seven files. The EF1002 warning that fires during the build is #122'sBackupService, not yours.///line and diffed6347907vsdb03ae9onQueryPlanTests.cs: byte-identical. Pure documentation, provably zero behavioural drift.The carried non-blocker, closed properly:
PlanFor's summary no longer claims it "fails loudly when the predicate matches no command or several." Now every sentence maps to a real line of the body — "first recorded command" ↔matches[0], "fails loudly when the predicate matches no command" ↔ theAssert.True(matches.Count > 0, …)with the captured-SQL dump, "several matches are expected — under AsSplitQuery the driving subquery repeats" ↔ the inline// The split query repeats…comment that already sat there. Doc and body finally tell the same story, and the commit message explains the AsSplitQuery reasoning. Silly little lying doc comment~ you couldn't stay, could you? ♡No new blockers, and here's why I'm confident rather than lazy: the only file touched is a test harness whose executable lines are unchanged from the state I approved in round 2 with four mutation probes. Comments don't need new coverage — the 5/5 QueryPlanTests still pin all four plans at this head.
✅ What I liked~
Local at
db03ae9(CI stale for this head — coverage bot ran 07:31, commit 07:35): build 0 errors (2 pre-existing warnings, both outside this PR), QueryPlanTests 5/5 pass [17s]. Round-2 verdict and all four mutation probes stand untouched.Merge-order caveat from round 2 still applies to whoever merges #118/#120 second: regenerate the migration + snapshot or
Model_And_Migrations_Agreewill catch the drift — that tripwire doing its job is a good thing~Automated review by Jibril · 2026-09-10
CI/CD: stale for head
db03ae9(bot comment covers6347907) · Local checks: build 0 errors, QueryPlanTests 5/5