multi: change sql db default max connections #10426
Conversation
Summary of ChangesHello @ziggie1984, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the default maximum connection settings for both PostgreSQL and SQLite databases across the application. The changes aim to improve database resource management and stability by setting more appropriate default limits, particularly considering the specific characteristics of each database system, such as SQLite's single-writer model, and updating test configurations to match these new defaults. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adjusts the default maximum database connections for SQL backends, which is a sensible change to optimize resource usage and prevent contention issues. The changes are consistent across the codebase. I've included a few suggestions to refactor duplicated constants for better maintainability.
| // defaultMaxConnsPostgres is the number of permitted active and idle | ||
| // connections. We want to limit this so it isn't unlimited. We use the | ||
| // same value for the number of idle connections as, this can speed up | ||
| // queries given a new connection doesn't need to be established each | ||
| // time. | ||
| defaultMaxConns = 25 | ||
| defaultMaxConnsPostgres = 10 | ||
|
|
||
| // defaultMaxConnsSqlite is the number of permitted active and idle | ||
| // connections. We want to limit this to 2 because SQLite allows for | ||
| // concurrent reads however writers are limited to 1 so we select 2 here | ||
| // to allow for concurrent reads but keep the number of connections low | ||
| // to avoid write contention. | ||
| defaultMaxConnsSqlite = 2 |
There was a problem hiding this comment.
To avoid duplicating these default values in the lncfg package, consider exporting these constants (e.g., DefaultMaxConnsPostgres, DefaultMaxConnsSqlite). The lncfg package can then reference them directly, ensuring consistency and making future changes easier. This change should be propagated to sqldb/postgres.go and sqldb/sqlite.go as well.
| // defaultMaxConnsPostgres is the number of permitted active and idle | |
| // connections. We want to limit this so it isn't unlimited. We use the | |
| // same value for the number of idle connections as, this can speed up | |
| // queries given a new connection doesn't need to be established each | |
| // time. | |
| defaultMaxConns = 25 | |
| defaultMaxConnsPostgres = 10 | |
| // defaultMaxConnsSqlite is the number of permitted active and idle | |
| // connections. We want to limit this to 2 because SQLite allows for | |
| // concurrent reads however writers are limited to 1 so we select 2 here | |
| // to allow for concurrent reads but keep the number of connections low | |
| // to avoid write contention. | |
| defaultMaxConnsSqlite = 2 | |
| // DefaultMaxConnsPostgres is the number of permitted active and idle | |
| // connections. We want to limit this so it isn't unlimited. We use the | |
| // same value for the number of idle connections as, this can speed up | |
| // queries given a new connection doesn't need to be established each | |
| // time. | |
| DefaultMaxConnsPostgres = 10 | |
| // DefaultMaxConnsSqlite is the number of permitted active and idle | |
| // connections. We want to limit this to 2 because SQLite allows for | |
| // concurrent reads however writers are limited to 1 so we select 2 here | |
| // to allow for concurrent reads but keep the number of connections low | |
| // to avoid write contention. | |
| DefaultMaxConnsSqlite = 2 |
| } | ||
|
|
||
| maxConns := defaultMaxConns | ||
| maxConns := defaultMaxConnsPostgres |
There was a problem hiding this comment.
To be consistent with exporting the default values from sqldb/config.go, this should use the exported constant.
| maxConns := defaultMaxConnsPostgres | |
| maxConns := DefaultMaxConnsPostgres |
|
|
||
| db.SetMaxOpenConns(defaultMaxConns) | ||
| db.SetMaxIdleConns(defaultMaxConns) | ||
| maxConns := defaultMaxConnsSqlite |
There was a problem hiding this comment.
To be consistent with exporting the default values from sqldb/config.go, this should use the exported constant.
| maxConns := defaultMaxConnsSqlite | |
| maxConns := DefaultMaxConnsSqlite |
| defaultPostgresMaxConnections = 10 | ||
| defaultSqliteMaxConnections = 2 |
There was a problem hiding this comment.
To avoid duplication and ensure consistency, these default values should be sourced from the sqldb package where they are also defined.
| defaultPostgresMaxConnections = 10 | |
| defaultSqliteMaxConnections = 2 | |
| defaultPostgresMaxConnections = sqldb.DefaultMaxConnsPostgres | |
| defaultSqliteMaxConnections = sqldb.DefaultMaxConnsSqlite |
ziggie1984
changed the title
ziggie1984
changed the title
1 participant
No description provided.