perf: PERF-10 add composite indexes for AuditLog, LlmRequest, Card (#846)#888
perf: PERF-10 add composite indexes for AuditLog, LlmRequest, Card (#846)#888Chris0Jeky merged 3 commits intomainfrom
Conversation
Adds composite HasIndex declarations for PERF-10 (#846): - AuditLog: (UserId, Timestamp) and (EntityId, Timestamp) - Card: (BoardId, ColumnId) LlmRequest (UserId, Status) already existed from 20260211082334.
Adds the three new composite indexes from PERF-10 (#846): - IX_AuditLogs_UserId_Timestamp - IX_AuditLogs_EntityId_Timestamp (board-scope analogue; AuditLog has no BoardId column, board queries filter via EntityId) - IX_Cards_BoardId_ColumnId (also covers the old IX_Cards_BoardId, which EF therefore drops) IX_LlmRequests_UserId_Status already existed since 20260211082334.
Adversarial self-reviewRe-read the migration, entity configs, and snapshot as an EF Core reviewer. Focused on: is the migration complete, are column names real, does it duplicate existing indexes, does the snapshot stay in sync, and does EF Core 8 accept the syntax used. Findings
CI statusWill monitor after posting. Acted on
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces performance optimizations by adding composite indexes to the AuditLogs and Cards tables to improve query efficiency. The migration file includes logic to drop redundant single-column indexes in favor of these new composite indexes. However, the migration appears incomplete as it lacks the necessary AlterColumn operation for the AutomationProposal.UpdatedAt concurrency token shown in the model snapshot. Additionally, there is a contradiction between the code comments in CardConfiguration.cs and the actual migration implementation regarding the retention of the IX_Cards_BoardId index.
| protected override void Up(MigrationBuilder migrationBuilder) | ||
| { | ||
| migrationBuilder.DropIndex( | ||
| name: "IX_Cards_BoardId", | ||
| table: "Cards"); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_Cards_BoardId_ColumnId", | ||
| table: "Cards", | ||
| columns: new[] { "BoardId", "ColumnId" }); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_AuditLogs_EntityId_Timestamp", | ||
| table: "AuditLogs", | ||
| columns: new[] { "EntityId", "Timestamp" }); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_AuditLogs_UserId_Timestamp", | ||
| table: "AuditLogs", | ||
| columns: new[] { "UserId", "Timestamp" }); | ||
| } |
There was a problem hiding this comment.
The Up method is missing operations that are reflected in the model snapshot and designer files, indicating an incomplete migration:
- Concurrency Token:
AutomationProposal.UpdatedAtis marked as a concurrency token in the snapshot (line 410) and designer (line 413), but there is noAlterColumnoperation here. This will cause drift and potential issues with future migrations. - Missing Index: The PR summary mentions adding
IX_LlmRequests_UserId_Status, but this index is not created in theUpmethod. It also doesn't appear as a change in the snapshot diff, suggesting it was already present or the configuration change was omitted.
Please regenerate the migration to ensure it captures all intended model changes.
| // PERF-10: composite index for board+column lookups (the default board | ||
| // load path filters by BoardId and then groups by ColumnId). SQLite can | ||
| // satisfy single-column filters on BoardId from this composite too, but | ||
| // we retain the existing IX_Cards_BoardId / IX_Cards_ColumnId indexes to | ||
| // avoid changing FK index conventions. |
There was a problem hiding this comment.
The comment contradicts the migration implementation. AddPerfIndexes.cs drops IX_Cards_BoardId at line 13, but this comment says it is retained. While dropping the redundant index is correct (as the composite index covers it), the documentation should be updated to reflect the actual state.
// PERF-10: composite index for board+column lookups (the default board
// load path filters by BoardId and then groups by ColumnId). SQLite can
// satisfy single-column filters on BoardId from this composite too, so
// the redundant single-column IX_Cards_BoardId is dropped in the migration
// while IX_Cards_ColumnId is retained.There was a problem hiding this comment.
Pull request overview
Adds EF Core model configuration + migration changes to improve query performance by introducing new composite indexes (primarily for AuditLogs and Cards) and updating the EF model snapshot accordingly.
Changes:
- Add composite index
IX_Cards_BoardId_ColumnIdand migration logic that replaces the priorIX_Cards_BoardIdsingle-column index. - Add composite indexes for AuditLogs:
IX_AuditLogs_UserId_TimestampandIX_AuditLogs_EntityId_Timestamp. - Update EF Core model snapshot and add the corresponding migration + designer output.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/Taskdeck.Infrastructure/Persistence/Configurations/CardConfiguration.cs | Adds composite Cards index configuration + explanatory PERF-10 comment. |
| backend/src/Taskdeck.Infrastructure/Persistence/Configurations/AuditLogConfiguration.cs | Adds composite AuditLogs indexes for user and entity history queries. |
| backend/src/Taskdeck.Infrastructure/Migrations/TaskdeckDbContextModelSnapshot.cs | Snapshot updated to reflect new composite indexes (and removal of Cards BoardId index). |
| backend/src/Taskdeck.Infrastructure/Migrations/20260416161303_AddPerfIndexes.cs | Migration creates the new indexes and drops IX_Cards_BoardId. |
| backend/src/Taskdeck.Infrastructure/Migrations/20260416161303_AddPerfIndexes.Designer.cs | Generated designer reflecting the updated model. |
Files not reviewed (1)
- backend/src/Taskdeck.Infrastructure/Migrations/20260416161303_AddPerfIndexes.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // satisfy single-column filters on BoardId from this composite too, but | ||
| // we retain the existing IX_Cards_BoardId / IX_Cards_ColumnId indexes to | ||
| // avoid changing FK index conventions. |
There was a problem hiding this comment.
The comment says we “retain the existing IX_Cards_BoardId / IX_Cards_ColumnId indexes”, but this change set actually removes the single-column BoardId index (see AddPerfIndexes migration + snapshot). Either update this comment to reflect the new indexing strategy (composite replaces BoardId index), or explicitly keep the BoardId index in the model/migration if that’s the intent.
| // satisfy single-column filters on BoardId from this composite too, but | |
| // we retain the existing IX_Cards_BoardId / IX_Cards_ColumnId indexes to | |
| // avoid changing FK index conventions. | |
| // also satisfy single-column filters on BoardId from the leftmost prefix | |
| // of this composite index. We do not add a separate BoardId index here, | |
| // and we leave FK convention indexes such as ColumnId unchanged. |
| migrationBuilder.DropIndex( | ||
| name: "IX_Cards_BoardId", | ||
| table: "Cards"); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_Cards_BoardId_ColumnId", | ||
| table: "Cards", | ||
| columns: new[] { "BoardId", "ColumnId" }); |
There was a problem hiding this comment.
Up() drops the existing IX_Cards_BoardId index. That contradicts the rationale comment in CardConfiguration (and the PR description) which says the existing BoardId index is retained. If the intent is only to add a composite index, avoid dropping IX_Cards_BoardId; if the intent is to replace it, please update the PR description/comments to match so consumers don’t assume both indexes exist.
| // Note: AuditLog has no BoardId column; board scope is resolved via | ||
| // EntityId (see AuditLogRepository.GetByBoardAsync). A composite on | ||
| // (EntityId, Timestamp) accelerates that ORDER BY Timestamp DESC pattern | ||
| // and serves as the board-scoped analogue named for in PERF-10. | ||
| builder.HasIndex(al => new { al.EntityId, al.Timestamp }) | ||
| .HasDatabaseName("IX_AuditLogs_EntityId_Timestamp"); |
There was a problem hiding this comment.
The PR/issue summary calls for a board-scoped composite index (BoardId, Timestamp), but the schema/config adds (EntityId, Timestamp) named IX_AuditLogs_EntityId_Timestamp instead. If board scoping is intentionally via EntityId, please align the PR description/issue acceptance criteria and consider whether the index/name should explicitly reflect EntityId semantics to avoid confusion; otherwise, implement the intended BoardId-based index (which would require adding a BoardId column).
| migrationBuilder.CreateIndex( | ||
| name: "IX_Cards_BoardId_ColumnId", | ||
| table: "Cards", | ||
| columns: new[] { "BoardId", "ColumnId" }); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_AuditLogs_EntityId_Timestamp", | ||
| table: "AuditLogs", | ||
| columns: new[] { "EntityId", "Timestamp" }); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_AuditLogs_UserId_Timestamp", | ||
| table: "AuditLogs", | ||
| columns: new[] { "UserId", "Timestamp" }); |
There was a problem hiding this comment.
This migration is described as adding four composite indexes (including LlmRequests UserId+Status and AuditLogs BoardId+Timestamp), but Up() only creates three (Cards BoardId+ColumnId, AuditLogs EntityId+Timestamp, AuditLogs UserId+Timestamp) and does not touch LlmRequests. If the LlmRequests index already existed previously, update the PR description accordingly; if it’s still required for some environments, it should be created here explicitly.
Prior comment said the single-column IX_Cards_BoardId was retained, but the AddPerfIndexes migration actually drops it in favor of the composite IX_Cards_BoardId_ColumnId (which SQLite uses for leftmost-prefix BoardId scans). The FK constraint remains enforced by the FOREIGN KEY schema itself, and IX_Cards_ColumnId is untouched. Update the comment to match reality. Addresses gemini-code-assist + Copilot review on PR #888.
Bot-findings triage + fresh adversarial passFixes committed
Gemini high-priority flag (rejected as false positive, with rationale)
This is incorrect for EF Core 8 + SQLite:
Fresh adversarial findings (all cleared)
Final CI status (before fix push)All 14 required checks green on |
Adds a delivery entry for the 8 PROD-00 PRs merged on 2026-04-16 (#884 SEC-28, #885 DOC-06, #887 DOC-07, #886 PERF-09, #888 PERF-10, #889 OPS-29, #890 FE-15, #891 FE-14) with round-2 adversarial review findings: BREACH JWT-in-body correction (compression level Optimal -> Fastest), IPv6/IPv4 healthcheck fix, null-throw sentinel fix, skipRetry opt-out for baseline tests, setpriv entrypoint for upgrade-safe volume ownership. Also bumps the Last Updated date.
…layer error coverage Adds a PROD-00 Production-Readiness Round-2 Wave section covering: - ResponseCompressionApiTests (#886, +3 tests) - migration-only context for composite DB indexes (#888) - container hardening verification (no unit tests, docker inspect path) - HTTP retry with backoff tests + skipRetry opt-out pattern for future test authors (#890) - ErrorBoundary + errorReporting tests + three-layer error coverage pattern documenting outer/inner/window layers (#891) Updates Current Verified Totals to reflect the new test deltas.
Adds a delivery entry for the 8 PROD-00 PRs merged on 2026-04-16 (#884 SEC-28, #885 DOC-06, #887 DOC-07, #886 PERF-09, #888 PERF-10, #889 OPS-29, #890 FE-15, #891 FE-14) with round-2 adversarial review findings: BREACH JWT-in-body correction (compression level Optimal -> Fastest), IPv6/IPv4 healthcheck fix, null-throw sentinel fix, skipRetry opt-out for baseline tests, setpriv entrypoint for upgrade-safe volume ownership. Also bumps the Last Updated date.
…layer error coverage Adds a PROD-00 Production-Readiness Round-2 Wave section covering: - ResponseCompressionApiTests (#886, +3 tests) - migration-only context for composite DB indexes (#888) - container hardening verification (no unit tests, docker inspect path) - HTTP retry with backoff tests + skipRetry opt-out pattern for future test authors (#890) - ErrorBoundary + errorReporting tests + three-layer error coverage pattern documenting outer/inner/window layers (#891) Updates Current Verified Totals to reflect the new test deltas.
Summary
Adds composite indexes via EF Core migration (
20260416161303_AddPerfIndexes):IX_Cards_BoardId_ColumnIdonCards (BoardId, ColumnId)— replaces the single-columnIX_Cards_BoardId, which SQLite can satisfy via the leftmost prefix of the composite. TheFK_Cards_Boards_BoardIdconstraint remains enforced by the FOREIGN KEY schema itself (not the index).IX_Cards_ColumnIdis retained.IX_AuditLogs_UserId_TimestamponAuditLogs (UserId, Timestamp)— acceleratesAuditLogRepository.GetByUserAsync(filters byUserIdand orders byTimestamp DESC).IX_AuditLogs_EntityId_TimestamponAuditLogs (EntityId, Timestamp)— acceleratesAuditLogRepository.GetByBoardAsync.Deviations from the issue AC
(BoardId, Timestamp)→(EntityId, Timestamp):AuditLoghas noBoardIdcolumn; board scope is resolved polymorphically viaEntityId. The composite on(EntityId, Timestamp)is the functional analogue and serves the same query path. Adding a literalBoardIdcolumn was out of scope for PERF-10.IX_LlmRequests_UserId_Statusnot created: Already present pre-PR (seeLlmRequestConfiguration.cs:59,builder.HasIndex(lr => new { lr.UserId, lr.Status })). Re-declaring would create a duplicateIX_LlmRequests_UserId_Status1.Ordering semantics (SQLite + EF Core 8)
Descending-timestamp ordering on an index requires EF Core 9 (
IsDescending()), which the repo does not use. The indexes are declared unordered; SQLite's query planner performs a backward range scan forORDER BY Timestamp DESCover the composite, which is acceptable for local-first SQLite workloads.Snapshot drift picked up
The regenerated snapshot also records
IsConcurrencyToken()onAutomationProposal.UpdatedAt, which was already declared in the entity configuration (main commit9f7dc936) but absent from the previous snapshot. This is a model-only annotation (no schema change on SQLite); noAlterColumnis required.Closes #846
Test plan