improve: speed up checking channels for changed messages - #214
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 10, 2026, 4:15 PM ET / 20:15 UTC. ClawSweeper reviewWhat this changesAdds a SQLite index that speeds per-channel changed-message queries, with tests for fresh databases and existing archive upgrades. Merge readiness⛔ Blocked before merge - 3 items remain The optimization remains useful and is absent from main and v0.14.1. No concrete correctness defect was found; the outstanding choice is whether to accept the documented, unmeasured large-archive upgrade cost. Likely related people: steipete and vkehfdl1 are unverified routing candidates, with low confidence. Priority: P2 Review scores
Verification
How this fits togetherDiscrawl stores archived Discord messages in SQLite for CLI queries and external consumers. The added index orders each channel’s records by update time and message ID, helping consumers retrieve incremental changes. flowchart LR
A[Archived messages] --> B[SQLite archive]
C[Writable archive open] --> D{Index exists?}
D -->|No| E[Build update-time index]
D -->|Yes| B
E --> B
F[Channel and update cursor] --> B
B --> G[Ordered changed messages]
Decision needed
Why: The data-preservation tests support structural compatibility, but synthetic read timings do not establish an acceptable operational cost for existing large archives. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the additive index and existing query contracts, with representative archive-upgrade costs measured or explicitly accepted before landing. Do we have a high-confidence way to reproduce the issue? Not applicable as a correctness-bug reproduction: this is a performance optimization supported by source inspection and submitted synthetic timings; no runtime benchmark was executed during review. Is this the best way to solve the issue? Yes, the additive covering index follows the existing maintenance pattern and preserves creation-time queries; the remaining question is its operational cost on large archives. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 5ef4faad55d0. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Additional instructions
MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.
What Problem This Solves
Agents and other integrations pulling incremental data from an archive need to ask, "Which messages in this channel changed since my last check?" That includes older messages edited or otherwise updated later. On channels with substantial history, finding the latest update or retrieving a small batch of changed messages can become unnecessarily expensive, slowing repeated polling as the archive grows.
Why This Change Was Made
Add a covering index on
messages(channel_id, updated_at, id)so per-channel updated-time queries can use the required ordering without a temporary sort. Ensure it exists through the existing initialization and index-maintenance paths, including when reopening an already-versioned archive, while retaining the existing indexes and schema version.User Impact
Agents and read-only archive consumers can find the latest update cursor and retrieve changed-message batches with less database work. The existing channel/creation-time index continues to support reads ordered by when messages were originally posted; the additional index supports reads ordered by when archived records were last updated.
Existing records and command behavior are preserved. On an existing archive, the first writable open that runs index maintenance will build the index once. That work can delay the command, hold up other writers, and use additional disk space; the index also has ongoing storage and message-write costs, including for installations that do not use these queries.
Evidence
The query-plan test verifies that an incremental channel query uses the new index and does not require
USE TEMP B-TREE. An existing-archive regression removes the index from a populated current-schema database, reopens it, and verifies index creation while preserving the message, queued embedding job, and schema version.A synthetic in-memory SQLite 3.51.0 fixture used 100,000 rows, including 80,000 in the queried channel. Median read timings across 15 repetitions were:
Both query plans stopped using a temporary sort. The additional index occupied 2,797,568 bytes in that fixture. These are synthetic read measurements, not production latency or storage estimates; large-archive index-build time and write-throughput impact have not yet been measured.
Validation:
go test -count=1 ./internal/storepassed.go vet ./internal/storepassed.git diff --checkpassed.