Set SQLite cache, mmap, and WAL synchronous pragmas - #1438
Conversation
|
🚨 SLOP COP 🚨 · ELI5: This pull request gives SQLite more memory and reduces slow disk work. I am the SlopCop. I am reviewing this pull request for security, code quality, architecture, performance, and product behavior. |
| sqlite.pragma(`cache_size = -${SQLITE_CACHE_SIZE_KIB}`); | ||
| sqlite.pragma(`mmap_size = ${SQLITE_MMAP_SIZE_BYTES}`); | ||
| sqlite.pragma(`busy_timeout = ${SQLITE_BUSY_TIMEOUT_MS}`); | ||
| sqlite.pragma("temp_store = MEMORY"); |
There was a problem hiding this comment.
🚨 slopcop/review — temp_store = MEMORY removes the normal disk spill path.
The main connection serves broad thread searches. That query groups and sorts matches before its final limit. Large matches can keep temporary indexes in process memory. The new page cache can also use 256 MiB. Please keep the default file store, or add a measured memory limit.
There was a problem hiding this comment.
Removed temp_store = MEMORY on 64d5e58. The connection keeps the default file store so large thread-search sorts can spill to disk.
| export { createConnection } from "./connection.js"; | ||
| export { | ||
| createConnection, | ||
| SQLITE_BUSY_TIMEOUT_MS, |
There was a problem hiding this comment.
🚨 slopcop/review — These exports add internal tuning policy to the package API.
No production code imports these constants. The test already imports them from connection.ts. Please remove the three root exports. This change will keep the public contract small.
There was a problem hiding this comment.
Removed the three root exports on 64d5e58. The test still imports the constants from connection.ts.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: This change gives SQLite more memory and reduces disk waits. It can run faster, but memory-only scratch work can grow too much.
I found two items:
- Medium:
temp_store = MEMORYremoves the disk spill path for temporary indexes. Broad thread searches can create data-dependent temporary work. Keep the default file store, or add a measured memory limit. - Low: The root package exports three tuning constants that no production code uses. Keep this policy private to the connection module.
The security review found no injection issue. All new pragma values use fixed constants. The NORMAL durability loss is explicit and matches SQLite behavior. A system failure can remove recent commits, but an application failure will not.
The architecture scan found one main database connection path. Plugin databases use a separate policy, so a shared setup helper would mix different contracts. The 5-second timeout already matches the better-sqlite3 default. Thus, this line does not add a new delay.
Validation passed:
- The
@bb/dbtype check passed. - All 380
@bb/dbtests passed. - All current GitHub checks passed.
- The dev server started from this commit.
- A browser opened the app and showed the empty thread view.
I posted two inline comments. I used a comment review only.
Give the 1.9 GB server database a 256 MiB page cache, 1 GiB mmap window, WAL-normal durability, a 5s busy timeout, and memory temp storage so cold page reads and per-commit fsync stop stalling the event loop.
temp_store=MEMORY removed the spill path for large thread-search sorts. Leave the default file store. Also drop the unused root exports so the tuning values stay private to the connection module.
46f753d to
64d5e58
Compare
Why
better-sqlite3is synchronous and every query runs on the server event loop. The live database is ~1.9 GB with a 2 MiB page cache, no mmap, andsynchronous = FULL(fsync on every commit). Cold page reads and per-commit fsync are the main source of sub-second stalls, not a missing index.This is layer 2 of the event-loop stall stack. It depends on layer 1 (#1437) so the new pragmas can be confirmed from stall and slow-query logs.
What
createConnectionnow sets:cache_size-262144(256 MiB)synchronousNORMALmmap_size1073741824(1 GiB)pread+ copy on every pagebusy_timeout5000temp_storeMEMORYwal_autocheckpointis left unchanged. A 38 MB WAL is a reader-starvation problem, not an autocheckpoint setting problem.Test plan
@bb/dbtypecheck + full test suitePrerequisite: #1437.