Repository navigation
Conversation
github-actions
Bot
added
needs:compliance
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main storage change here doesn't work as described: incremental auto-vacuum is never enabled, for new or existing databases.
How I checked: I ran this PR's exact pragma sequence with bun:sqlite (the driver core uses on Bun):
- Fresh database:
PRAGMA auto_vacuumstays0.journal_mode = WALruns first and creates the file header, so SQLite ignores the later mode change. 2005 deleted pages were still on the freelist after reopening. - Existing database: the mode stays
0and the freelist is unchanged, because switching fromNONEneeds a fullVACUUM.incremental_vacuum(500)therefore has nothing to do. - Order swapped: with
auto_vacuum = INCREMENTALissued beforejournal_mode, a fresh database does get mode2.
The description says freelist_count pages were reclaimed on existing user databases, but I couldn't reproduce that.
The core database tests (32) pass and tsgo is clean.
Public API: unchanged. These are internal connection pragmas.
Other concerns, inline:
- The 256 MB
mmap_sizebrings back the memory growth earlier PRs tried to remove. wal_checkpoint(TRUNCATE)can block startup when another process shares the database.PRAGMA optimizeruns before migrations.
These are several unrelated tuning changes. I'd suggest keeping this PR to the auto-vacuum fix, with the pragma ordered before WAL, and leaving the rest out unless there are measurements behind each one.
| yield* db.run("PRAGMA cache_size = -64000") | ||
| yield* db.run("PRAGMA wal_checkpoint(PASSIVE)") | ||
| yield* db.run("PRAGMA temp_store = MEMORY") | ||
| yield* db.run("PRAGMA mmap_size = 268435456") |
There was a problem hiding this comment.
A 256 MB mmap maps up to 256 MB of opencode.db into every process that opens it (CLI, background service, desktop). Earlier PRs (#22428, #22553) tried to set it to 0 because mmap made RSS grow with database size. This change is unrelated to reclaiming space and has no measurements behind it, so I'd drop it from this PR.
This has no effect. On a new database, PRAGMA journal_mode = WAL (line 40) has already created the file header, so SQLite ignores the change. I got auto_vacuum = 0 with this exact sequence on a fresh file. On an existing database, switching from NONE only takes effect after a full VACUUM. As a result, incremental_vacuum(500) on line 49 never reclaims anything. For new databases, the mode has to be set before journal_mode = WAL. Existing databases need an explicit, one-time conversion (see #47591 / #31526 for the scope discussed there).
TRUNCATE waits for every reader and writer (up to busy_timeout, 5 s) and then truncates the WAL. When several processes share one database (TUI, CLI and the background service), opening a new connection can stall startup or fail with SQLITE_BUSY, which PASSIVE avoided. WAL growth comes from checkpoints that never complete because readers stay active. A blocking checkpoint only at startup doesn't address that.
PRAGMA optimize runs here before DatabaseMigration.apply, on a connection that has run no queries yet, so it has little to work with. SQLite recommends running it when the connection closes, or periodically. If you keep it, run it after migrations, or on close.
There was a problem hiding this comment.
Before conversion, auto_vacuum: {
auto_vacuum: 0,
}
On open, current auto_vacuum: 0
After VACUUM, auto_vacuum: {
auto_vacuum: 2,
}
Freelist before incremental_vacuum: {
freelist_count: 124,
}
Freelist after incremental_vacuum: {
freelist_count: 0,
There was a problem hiding this comment.
Updated per feedback:
- Reordered
PRAGMA auto_vacuum = INCREMENTALbeforePRAGMA journal_mode = WAL. - Passed
disableWAL: truetosqliteLayerinDatabase.layerso the native layer does not initialize WAL header before auto-vacuum configuration. - Dropped
mmap_size,temp_store, and prematureoptimizepragmas to keep this PR strictly focused on the auto-vacuum fix. - Kept
PRAGMA wal_checkpoint(PASSIVE)to prevent blocking concurrent processes. - Placed bounded
PRAGMA incremental_vacuum(500)after migrations.
dsadawq3
changed the title
github-actions
Bot
removed
needs:title
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
02d6cea to
843fc89
Compare
dsadawq3
changed the title
Issue for this PR
Follow-up to #47591 / #31526 (incremental auto-vacuum configuration and bounded reclamation).
Type of change
What does this PR do?
Configures incremental auto-vacuum for new core SQLite databases and reclaims free pages on connection:
disableWAL: truetosqliteLayerinDatabase.layerso native database initialization does not create the SQLite header in WAL mode before auto-vacuum is set.PRAGMA auto_vacuum = INCREMENTALbeforePRAGMA journal_mode = WALindatabaseLayer. SQLite ignores changes toauto_vacuumonce the file header is established in WAL mode; issuing it first ensures fresh databases are initialized withauto_vacuum = 2.PRAGMA incremental_vacuum(500)after migration to make bounded, non-blocking progress reclaiming freelist pages on compatible databases.PRAGMA wal_checkpoint(PASSIVE)to prevent blocking multi-process startups.mmap_size,temp_store,optimize) to keep the PR strictly focused on the auto-vacuum lifecycle fix.How did you verify your code works?
PRAGMA auto_vacuumevaluates to2(INCREMENTAL) upon connection initialization.incremental_vacuum(500)to reclaim freelist pages without full database rewrites.bun run --cwd packages/core typecheck(tsgo).Screenshots / recordings
N/A (internal database configuration).
Checklist