fix(sqlite): stop getTxByOffset misses from scanning the offset index to the end#818
Conversation
… to the end selectStableTransactionOffsetById kept the span predicate ((offset - data_size) < @offset) inside the index scan. On a HIT the first index row matches and the query returns in ~10ms, but on a MISS -- an offset in a coverage gap (format-1 tx, unpopulated region) -- no row can ever match, and SQLite walked stable_transactions_offset_idx from @offset to the end of the table with a table-row fetch per entry before returning empty. On a production-sized DB that is tens of seconds to minutes per miss (observed up to 110s via the slow-op log), each one pinning a core read worker and holding the requesting peer's socket, which cascades into outbound socket-pool saturation. Since weave data regions never overlap, the first format-2 data_size > 0 row with offset >= @offset is the only possible spanner. Restructure the statement to fetch exactly that row in an inner LIMIT 1 subquery and apply the span check outside, making a miss cost the same single index probe as a hit. The partial-index predicates (format = 2 AND data_size > 0) must stay inside the inner query verbatim: stable_transactions_offset_idx is a partial index and SQLite only uses it when the query's WHERE clause implies the index's WHERE clause. Hoisting them out silently degrades to a full table scan -- the SQL comment and the new plan-assertion test both guard this. Validated against a production-sized core DB (read-only): hit unchanged (~10ms, same row); previously-pathological misses (logged 23-110s) now ~10ms; the plan is a single SEARCH via stable_transactions_offset_idx. Adds a regression test covering hit, gap miss, format-1-spanned miss, beyond-end miss, and an EXPLAIN QUERY PLAN assertion that the statement probes stable_transactions_offset_idx and never scans the table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
URL.pathname is URL-encoded and non-native on some platforms; fileURLToPath yields a correct filesystem path everywhere. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe stable transaction offset lookup query now selects a single partial-index-backed candidate before applying the span check, and the standalone SQLite tests add regression coverage for hit and miss behavior plus query-plan validation. ChangesOffset Query Fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/database/standalone-sqlite.test.ts (1)
422-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
fileURLToPathoverURL(...).pathnamefor the SQL directory.
new URL('./sql/core', import.meta.url).pathnamereturns a URL-encoded, non-native path (leading-slash drive paths on Windows,%20for spaces), whichfs.readdirSyncinloadSqlwill mishandle. UsingfileURLToPathyields a correct filesystem path across platforms.Also worth confirming the
.sqlfiles resolve at test runtime (i.e., tests execute againstsrc/, not a compileddist/wheresql/core/*.sqlmay not be emitted), otherwiseloadSqlreturns an empty map and the assertion at Line 424 fails.♻️ Proposed change
- const offsetsSqlDir = new URL('./sql/core', import.meta.url).pathname; + const offsetsSqlDir = fileURLToPath(new URL('./sql/core', import.meta.url));Add the import (adjust to existing
node:urlimport if present):+import { fileURLToPath } from 'node:url';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/standalone-sqlite.test.ts` around lines 422 - 423, The SQL directory path in the standalone SQLite test is built with URL.pathname, which can produce an invalid filesystem path and break loadSql on some platforms. Update the test setup around loadSql/offsetsSqlDir to use fileURLToPath with import.meta.url so the path is native and decoded, and make sure the test is resolving against the source sql/core directory at runtime so the selectStableTransactionOffsetById lookup still finds the .sql file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/database/standalone-sqlite.test.ts`:
- Around line 422-423: The SQL directory path in the standalone SQLite test is
built with URL.pathname, which can produce an invalid filesystem path and break
loadSql on some platforms. Update the test setup around loadSql/offsetsSqlDir to
use fileURLToPath with import.meta.url so the path is native and decoded, and
make sure the test is resolving against the source sql/core directory at runtime
so the selectStableTransactionOffsetById lookup still finds the .sql file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2767e95b-6929-4a41-8bbd-ede8b79f38ae
📒 Files selected for processing (2)
src/database/sql/core/offsets.sqlsrc/database/standalone-sqlite.test.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #818 +/- ##
========================================
Coverage 78.75% 78.75%
========================================
Files 133 133
Lines 50880 50880
Branches 3834 3835 +1
========================================
Hits 40072 40072
Misses 10757 10757
Partials 51 51 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai Addressed the nitpick in f53145f — switched to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
sed -n '1,60p' src/database/standalone-sqlite.test.ts | head -80
rg -n "fileURLToPath|pathname" src/database/standalone-sqlite.test.ts src/database/standalone-sqlite.tsLength of output: 2388
On the second point, that's a fair call. |
|
Disjointness assumption validated against production data. The rewrite assumes tx data regions in On this dataset the new statement is therefore provably input-equivalent to the old one (identical hits and misses for every possible offset) — minus the pathological miss-scan cost. |
1 participant
Motivation
The SQLite slow-op log added in #817 captured
getTxByOffsetcalls withqueueWaitMs=0butserviceMsof 23–110 seconds on a canary gateway, allduring retrieval-load episodes where peer socket pools pinned at their caps.
Re-running the captured offsets against a production-sized core DB showed every
slow call was a miss — an absolute weave offset in a coverage gap (format-1
tx or unpopulated region) — while hits consistently resolved in ~10ms.
Root cause:
selectStableTransactionOffsetByIdkeeps the span predicate(
(offset - data_size) < @offset) inside the index scan. On a hit the firstindex row matches immediately. On a miss no row can ever match, so SQLite walks
stable_transactions_offset_idxfrom@offsetto the end of the table — with atable-row fetch per entry — before returning empty. Each such miss pins a core
read worker for tens of seconds and holds the requesting peer's socket open,
cascading into outbound socket-pool saturation.
Fix
Weave data regions never overlap, so the first
format = 2, data_size > 0row with
offset >= @offsetis the only possible spanner — any other tx whoseregion ends at or past
@offsetstarts after that row's region ends. Thestatement now fetches exactly that row in an inner
LIMIT 1subquery andapplies the span check outside. A miss costs the same single index probe as a
hit.
Partial-index caveat (guarded in code + test):
stable_transactions_offset_idxis a partial index (
WHERE format = 2 AND data_size > 0). SQLite only uses itwhen the query's WHERE clause implies the index's WHERE clause, so both
predicates must remain inside the inner query verbatim — hoisting either one out
silently degrades to a full table scan. The SQL comment documents this and the
new test asserts the plan.
Validation
Against a production-sized core DB (read-only, exact offsets captured by the
slow-op log):
EXPLAIN QUERY PLANconfirms a singleSEARCH stable_transactions USING INDEX stable_transactions_offset_idx (offset>?).Tests
New regression test covering: hit, gap miss, format-1-spanned miss, beyond-end
miss, and an
EXPLAIN QUERY PLANassertion (statement loaded viasql-loader,not duplicated) that the plan probes
stable_transactions_offset_idxand neverscans
stable_transactions— this specifically catches the partial-indexmismatch, which timing on a small test DB cannot. Full
standalone-sqlite.test.tssuite: 69/69 pass.🤖 Generated with Claude Code