Repository navigation
Apply SQLite connection settings to every connection - #1677
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
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. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
ac45350 to
811ebbd
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
aaronbrethorst
added
the
skip-template-check
811ebbd to
ddbc37f
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
| // concurrent readers and a single writer. journal_mode=WAL is stored in the | ||
| // database file, so one PRAGMA applies it to every connection. That is why it | ||
| // stays a PRAGMA, while the connection-scoped settings ride the DSN. | ||
| func configureSQLitePerformance(ctx context.Context, db *sql.DB) error { |
There was a problem hiding this comment.
With temp_store=MEMORY gone, SQLite sort and temp-table spills (for example CREATE INDEX during import) go to SQLITE_TMPDIR/TMPDIR, which defaults to /tmp. This stack's own downloadDir (internal/gtfs/feed_file.go) avoids /tmp because it "may be held in memory (tmpfs)". On a host where /tmp is tmpfs, the spill still counts against the 512 MB memory limit, so the saving is lost. On a read-only root filesystem, the import fails with SQLITE_IOERR or "disk full". Consider pointing SQLite's temp directory at the database's directory (for example, set SQLITE_TMPDIR to filepath.Dir(DBPath) at startup) rather than leaving it to deployment docs.
Generated by Claude Code
There was a problem hiding this comment.
I don't think this one holds. SQLite's unix VFS tries SQLITE_TMPDIR, TMPDIR, then /var/tmp before /tmp (sqlite3-binding.c around line 46460 in mattn v1.14.52), and /var/tmp is normally on disk. It also reads those env vars once at OS init, so setting them inside createDB wouldn't be reliable. And before this PR, 24 of the 25 connections already used file-backed temp storage. A read-only root is a deployment setting (TMPDIR in the Dockerfile), so I'd rather handle that separately.
| // depend on how a driver's SQLite was compiled. A larger cache didn't lower | ||
| // latency under the k6 load test, because the operating system already caches | ||
| // the database file. | ||
| const connectionCacheKiB = 2000 |
There was a problem hiding this comment.
The 2 MB cache also applies to the connection that runs the static import: the bulk inserts and the index and layover builds. Before this change, that connection had a 64 MB cache whenever it happened to get the configured one. The PR measures one feed (NJ Transit bus). A larger feed with large indexes, such as a combined-agency feed, could get much slower to import with 2 MB, and import time isn't covered by k6. Consider a larger cache on the import transaction's connection only (PRAGMA cache_size on the *sql.Conn or *sql.Tx used for the import), or measuring import time on the largest feed you support.
Generated by Claude Code
There was a problem hiding this comment.
I measured this on King County Metro (1.03M stop_times, 190k shape points), two runs each. 2 MB: 9.30 s and 9.51 s, peak RSS 128-129 MiB. 64 MB: 10.32 s and 9.53 s, peak RSS 467-541 MiB. The bigger cache isn't faster and costs about 4x the memory, so I'm keeping 2 MB.
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
d8fbbbb to
e65003d
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
e65003d to
86b3c7e
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
86b3c7e to
3151173
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
3151173 to
d22498d
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
d22498d to
65c1dab
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
65c1dab to
65a0da9
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
65a0da9 to
1af0361
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
configureSQLitePerformance set cache_size and temp_store with db.ExecContext, which runs on one pooled connection. Both settings are per connection, so that one connection had a 64 MB page cache and in-memory temp tables while the other 24 used SQLite's defaults, and which settings a query or an import got depended on the connection it drew. After SetConnMaxLifetime closed it, no connection had them. cache_size now rides the DSN, as foreign_keys already does, so every connection gets it. It is 2 MB, the SQLite default that most connections already used: under the k6 load test against Sound Transit, a 2 MB, 8 MB and the old mixed setup all gave p95 latencies of 7-8 ms with no failures, because the operating system caches the database file. 25 connections can then hold at most about 50 MB of page cache, where applying 64 MB to every connection could have held 1.6 GB. temp_store=MEMORY is dropped, so every connection keeps SQLite's default and the import's large sorts don't build temp tables in memory. The pure-Go driver also gets the 5 s busy_timeout the CGo driver sets by default. journal_mode=WAL stays a PRAGMA, since it is stored in the database file. The pragma test now checks several pooled connections of a file database; it only passed before because an in-memory database has a single connection. Closes #1659
DSN and dsn were identical in driver_cgo.go and driver_pure.go, apart from the parameter string, and both copied the "?"/"&" separator logic. They now live once in config.go, and each driver file supplies only connectionParams, its spelling of the connection settings. A new connection setting then changes one line per driver.
connectionBusyTimeoutMs reached only the pure-Go DSN. The CGo driver got 5000 ms only because mattn/go-sqlite3 defaults busyTimeout to 5000, so the busy_timeout check on CGo tested that default, not the constant. If either changed, the two drivers would drift apart with no test failing. The CGo DSN now passes _busy_timeout from the constant. The DSN test builds the cache size from connectionCacheKiB too, rather than hard-coding -2000, so changing the cache size doesn't break a test about separators.
The connectionCacheKiB comment repeated the pool size of 25 from configureConnectionPool and gave the resulting page cache bound in MB, so a change to the pool limit would leave the comment wrong. The limit is now the maxOpenConnections constant, which configureConnectionPool uses and the comment names, and the comment no longer states a derived MB figure.
The function now only sets journal_mode=WAL, since the connection-scoped settings moved to the DSN, so its old name promised more than it does. createDB's error message and comment say WAL mode too.
TestSQLitePerformancePragmasApplied opened and held its connections with its own Conn call and a defer in the loop, while the other two pooled-connection tests use holdConnections. It now uses the helper too. The client closes through t.Cleanup, so it closes after the held connections rather than before them.
1af0361 to
d1f9795
Compare
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Fixes #1659. Part of #1657: steady-state memory on a 512 MB instance. Stacked on #1676.
What it does
cache_sizegoes in the DSN for both drivers (mattn_cache_size, modernc_pragma=cache_size(...)), at 2 MB, SQLite's built-in default.temp_store=MEMORYis dropped, so large sorts spill to temp files. The pure-Go driver gets the same 5 sbusy_timeoutthat mattn sets by default.journal_mode=WALstays a PRAGMA because it is stored in the database file.Root cause
configureSQLitePerformancesetcache_sizeandtemp_storewithdb.ExecContext, which runs on one pooled connection, and both settings are per connection. One connection had a 64 MB page cache and in-memory temp tables while the other 24 used SQLite's defaults; which one a query got was random; and afterSetConnMaxLifetime(5 min) closed that connection, no connection had them.Testing
mainbefore this change:TestSQLitePerformancePragmasApplied, run against the base branch (three pooled connections of a file database):TestImportMemory, at the previous PR in the stack and at this branch:inserting_stop_timespeak heapbuilding_block_layover_indexpeak heapFull per-stage output is in the
TestImportMemorylog; the table shows the stages this PR moves.Tests:
TestSQLitePerformancePragmasApplied: holds three pooled connections of a file database open at once and checkscache_size,busy_timeout,temp_storeandjournal_modeon each. The old version only passed because an in-memory database has a single connection.TestDSNCacheSizeReachesEveryConnection: opens the DSN with a 3000 KiB cache and checks-3000on three connections, so a dropped DSN parameter is caught (2000 KiB is also SQLite's default, which the first test cannot tell apart).TestDSNAppendsConnectionSettingsWithCorrectSeparator: golden DSN strings for a plain path and for a path that already has a query string.TestDSNWithExistingQueryOpens: the&form opens and the driver honoursforeign_keysandbusy_timeout.TestMemoryDatabaseConnectionSettings::memory:keepsforeign_keys,busy_timeoutandjournal_mode=memory.TestForeignKeysEnabledOnEveryPooledConnection: unchanged, now through the sharedholdConnectionshelper.The pure-Go
busy_timeoutassertion only discriminates under-tags purego, which CI does not run; it passes locally in both modes.Checklist
mainand passes on this branch.gtfsdb/(db.go,models.go,query.sql.go) ortestdata/openapi.yml(synced from upstream).r.Context()to DB/service calls, notcontext.Background().testdata/openapi.yml, has an issue open on OneBusAway/sdk-config, or is outside the SDK and the description says why.Notes for reviewers
Size. 156 added, 74 removed; 108 of the additions are tests, most of them holding several pooled connections open to check each one.
Why 2 MB. It bounds SQLite's page cache at about 50 MB across 25 connections; applying the old 64 MB to every connection could have held 1.6 GB. Earlier k6 runs against Sound Transit (up to 500 VUs) showed no latency gain from a bigger cache, because the OS already caches the database file.
Temp files. Without
temp_store=MEMORY, SQLite writes temp files toSQLITE_TMPDIR/TMPDIR(/tmpin the image, writable by themaglevuser). A deployment with a read-only root filesystem and no writable/tmpwould needSQLITE_TMPDIRpointed at the data volume.