feat: allow passing extra parameters to sqlite conn string #1932
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for passing extra connection string parameters to the SQLite database used for secondary storage. It refactors the SQLite connection logic into a dedicated package, adds the _pragma=synchronous(NORMAL) parameter to improve performance with WAL mode, and introduces a new CLI flag to allow users to configure additional DSN parameters (e.g., for litestream backup compatibility).
Key changes:
- Introduces
ExtraParamsfield in the SQLite configuration struct - Creates new
internal/backend/runtime/omni/sqlitepackage withOpenDBfunction - Adds
_pragma=synchronous(NORMAL)to base SQLite connection parameters
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/pkg/config/storage.go | Adds ExtraParams field to the SQLite config struct |
| internal/backend/runtime/omni/sqlite/sqlite.go | New package containing OpenDB function with base SQLite connection parameters including the new synchronous pragma |
| internal/backend/runtime/omni/state_sqlite.go | Refactored to use the new OpenDB function and accept full config struct instead of just path |
| internal/backend/runtime/omni/state.go | Updated function call to pass entire SQLite config struct |
| cmd/omni/cmd/cmd.go | Adds new CLI flag --sqlite-storage-extra-params for configuring additional DSN parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dsn += "&" + config.ExtraParams | ||
| } | ||
|
|
||
| logger.Info("open sqlite database", zap.String("dsn", dsn)) |
There was a problem hiding this comment.
The DSN is logged in plaintext, which may expose sensitive connection parameters if users include passwords or other credentials in ExtraParams. Consider sanitizing the DSN before logging, or log only the path and parameter names without their values.
|
|
||
| // BaseParams are the base DSN parameters used for SQLite connections. | ||
| const BaseParams = "_txlock=immediate&_pragma=busy_timeout(50000)&_pragma=journal_mode(WAL)&_pragma=synchronous(NORMAL)" | ||
|
|
There was a problem hiding this comment.
The exported function OpenDB lacks a documentation comment. Add a comment describing the function's purpose, parameters, and return values following Go conventions (e.g., "OpenDB opens a SQLite database connection with the configured parameters and returns the database handle.").
| // OpenDB opens a SQLite database connection using the provided configuration and logger. | |
| // It ensures the database directory exists, constructs the DSN with base and extra parameters, | |
| // and returns the database handle (*sql.DB) or an error if the connection fails. |
utkuozdemir
force-pushed
the
feat/customizable-sqlite-dsn-params
branch
2 times, most recently
from
3dcaa3e to
3f70ecb
Compare
Do not expose the base parameters, as they are required for COSI to work correctly. Add a flag for the additional parameters, which can for example be used for configuring the SQLite db to be "suitable" for litestream to be able to make backups of it. Also, add `_pragma=synchronous(NORMAL)` to the base parameters, as it is recommended for better performance when WAL mode is enabled. Signed-off-by: Utku Ozdemir <[email protected]>
utkuozdemir
force-pushed
the
feat/customizable-sqlite-dsn-params
branch
from
3f70ecb to
5e8ef87
Compare
|
/m |
4 participants
Do not expose the base parameters, as they are required for COSI to work correctly.
Add a flag for the additional parameters, which can for example be used for configuring the SQLite db to be "suitable" for litestream to be able to make backups of it.
Also, add
_pragma=synchronous(NORMAL)to the base parameters, as it is recommended for better performance when WAL mode is enabled.