feat(database): expose additional pgxpool connection pool configurati… #2609
Conversation
WalkthroughThis PR adds new database pool and tuning fields (max_conns, min_conns, min_idle_conns, health_check_period, max_conn_lifetime_jitter, connect_timeout), deprecates legacy fields (max_open_connections, max_idle_connections), and updates configs, CLI flags, factories, postgres options, tests, and docs with backward-compatibility mappings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Config as Config Source (file / flags / env)
participant Factory as internal/factories/database.go
participant Postgres as pkg/database/postgres
Note over Config: New fields: max_conns, min_conns, min_idle_conns,\nhealth_check_period, max_conn_lifetime_jitter, connect_timeout
Config->>Factory: provide Database config
Factory->>Factory: build opts[] from new fields\napply legacy fallbacks if fields unset
Factory->>Postgres: New / NewWithSeparateURIs + opts...
Postgres->>Postgres: apply opts to write/read pools\n(min/max/idle/health/connect/jitter)
alt new fields set
Postgres->>pgxpool: set MaxConns/MinConns/MinIdleConns etc.
else legacy fields only
Postgres->>pgxpool: map legacy max_open_connections/max_idle_connections
end
Postgres-->>Factory: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/database/postgres/consts.go (1)
4-13: Defaults and 0-sentinel semantics look good; consider tightening commentsThe new pool-related defaults are consistent and line up with
DefaultConfig(e.g., max data per write, retries, watch buffer size). Using0as a sentinel to mean “use pgx/pgxpool default” is also coherent acrossMaxConns,HealthCheckPeriod,MaxConnLifetimeJitter, andConnectTimeout.To avoid any misread that
0itself is an “override” value, you could tweak the comments to explicitly say “non-zero overrides pgx/pgxpool defaults” instead of “Set explicitly to override”.internal/factories/database.go (1)
22-26: Pool option wiring is sound; consider tightening back-compat branchesThe postgres branch correctly:
- Builds a shared
optsslice,- Prefers
MaxConnsoverMaxOpenConnections,- Adds
MinConns/MinIdleConnsand the new timing knobs only when non-zero.Two small refinements would make the behavior closer to the comments and reduce redundant settings:
- Guard
MaxOpenConnectionson it being > 0Right now you always fall back to
MaxOpenConnectionswhenMaxConns == 0, even ifMaxOpenConnectionsis also 0. GivenMaxOpenConnectionsis deprecated and 0 typically means “don’t override / use defaults”, you can avoid setting it at all when it’s zero:- if conf.MaxConns > 0 { - opts = append(opts, PQDatabase.MaxConns(conf.MaxConns)) - } else { - // Backward compatibility: if MaxConns is not set, use MaxOpenConnections - opts = append(opts, PQDatabase.MaxOpenConnections(conf.MaxOpenConnections)) - } + if conf.MaxConns > 0 { + opts = append(opts, PQDatabase.MaxConns(conf.MaxConns)) + } else if conf.MaxOpenConnections > 0 { + // Backward compatibility: if MaxConns is not set, use MaxOpenConnections + opts = append(opts, PQDatabase.MaxOpenConnections(conf.MaxOpenConnections)) + }
- Make the “MinIdleConns takes precedence over MaxIdleConnections” comment true at this layer
Currently both options can be appended when both are >0, and any precedence is left to the postgres layer. To reflect the comment and simplify behavior, you can only fall back to
MaxIdleConnectionswhenMinIdleConnsisn’t set:- // Kept for backward compatibility - if conf.MaxIdleConnections > 0 { - opts = append(opts, PQDatabase.MaxIdleConnections(conf.MaxIdleConnections)) - } - - // Add MinIdleConns if set (takes precedence over MaxIdleConnections) - if conf.MinIdleConns > 0 { - opts = append(opts, PQDatabase.MinIdleConns(conf.MinIdleConns)) - } + // Add MinIdleConns if set (preferred) + if conf.MinIdleConns > 0 { + opts = append(opts, PQDatabase.MinIdleConns(conf.MinIdleConns)) + } else if conf.MaxIdleConnections > 0 { + // Backward compatibility: fall back to max_idle_connections when min_idle_conns is unset + opts = append(opts, PQDatabase.MaxIdleConnections(conf.MaxIdleConnections)) + }These changes keep the same capabilities while making the precedence and back-compat story explicit and easier to reason about.
Also applies to: 39-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/performance-test/values.yaml(1 hunks)docs/setting-up/configuration.mdx(4 hunks)example.config.yaml(1 hunks)internal/config/config.go(2 hunks)internal/factories/database.go(2 hunks)pkg/cmd/config.go(2 hunks)pkg/cmd/flags/serve.go(2 hunks)pkg/cmd/serve.go(1 hunks)pkg/database/postgres/consts.go(1 hunks)pkg/database/postgres/options.go(2 hunks)pkg/database/postgres/postgres.go(4 hunks)pkg/database/postgres/postgres_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
internal/factories/database.go (2)
pkg/database/postgres/options.go (14)
Option(8-8)MaxConnectionIdleTime(53-57)MaxConnectionLifeTime(60-64)WatchBufferSize(93-97)MaxDataPerWrite(87-91)MaxRetries(99-103)MinConns(36-40)MaxConns(18-22)MaxOpenConnections(13-15)MaxIdleConnections(28-32)MinIdleConns(46-50)HealthCheckPeriod(67-71)MaxConnLifetimeJitter(74-78)ConnectTimeout(81-85)pkg/database/postgres/postgres.go (2)
NewWithSeparateURIs(48-50)New(43-45)
pkg/cmd/config.go (2)
internal/config/config.go (1)
Database(160-184)pkg/database/postgres/options.go (9)
MaxConns(18-22)MaxOpenConnections(13-15)MaxIdleConnections(28-32)MinConns(36-40)MinIdleConns(46-50)MaxConnectionIdleTime(53-57)HealthCheckPeriod(67-71)MaxConnLifetimeJitter(74-78)ConnectTimeout(81-85)
pkg/cmd/serve.go (2)
internal/config/config.go (1)
Database(160-184)pkg/database/postgres/options.go (9)
MaxConns(18-22)MaxOpenConnections(13-15)MaxIdleConnections(28-32)MinConns(36-40)MinIdleConns(46-50)MaxConnectionIdleTime(53-57)HealthCheckPeriod(67-71)MaxConnLifetimeJitter(74-78)ConnectTimeout(81-85)
internal/config/config.go (1)
pkg/database/postgres/options.go (9)
MaxConns(18-22)MaxOpenConnections(13-15)MaxIdleConnections(28-32)MinConns(36-40)MinIdleConns(46-50)MaxConnectionIdleTime(53-57)HealthCheckPeriod(67-71)MaxConnLifetimeJitter(74-78)ConnectTimeout(81-85)
pkg/database/postgres/options.go (1)
pkg/database/postgres/postgres.go (1)
Postgres(22-40)
pkg/database/postgres/postgres_test.go (2)
pkg/database/postgres/postgres.go (1)
Postgres(22-40)pkg/database/postgres/options.go (8)
MaxConns(18-22)MinConns(36-40)MinIdleConns(46-50)HealthCheckPeriod(67-71)MaxConnLifetimeJitter(74-78)ConnectTimeout(81-85)MaxOpenConnections(13-15)MaxIdleConnections(28-32)
pkg/database/postgres/postgres.go (1)
pkg/database/postgres/options.go (6)
MinConns(36-40)MinIdleConns(46-50)MaxConns(18-22)MaxConnLifetimeJitter(74-78)HealthCheckPeriod(67-71)ConnectTimeout(81-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test with Coverage
- GitHub Check: Analyze (go)
- GitHub Check: Test with Coverage
🔇 Additional comments (9)
pkg/cmd/serve.go (1)
123-127: New database pool flags and deprecation wiring look consistentThe added flags cleanly expose the new pool settings and keep the old ones with clear deprecation messages. Names and descriptions match the
config.Databasefields and the postgres options surface.Assuming
flags.RegisterServeFlagsis updated to bind these to Viper/env keys, this part looks solid.Also applies to: 130-132
pkg/cmd/flags/serve.go (1)
468-473: New DB pool flag/env bindings look consistentThe new Viper bindings for
database.max_conns,min_conns,min_idle_conns,health_check_period,max_conn_lifetime_jitter, andconnect_timeoutare named consistently with the flags and environment variables used elsewhere, while keeping the deprecated fields wired for backward compatibility. No issues from this layer.Also applies to: 503-536
pkg/cmd/config.go (1)
93-97: CLI/config surface for new pool options is coherentThe new flags and inspection rows for
database-max-conns,database-min-conns,database-min-idle-conns,database-health-check-period,database-max-conn-lifetime-jitter, anddatabase-connect-timeoutare wired correctly to the config struct and envs, and the deprecation notes for the legacy flags are clear. Looks good.Also applies to: 100-102, 223-227, 230-232
pkg/database/postgres/postgres_test.go (1)
70-72: Tests correctly cover new options and BC semanticsThe added tests for
MaxConns,MinConns,MinIdleConns,HealthCheckPeriod,MaxConnLifetimeJitter,ConnectTimeout, plus the deprecated wrappers (MaxOpenConnections,MaxIdleConnections), give good coverage of the option setters. The “Backward Compatibility” cases accurately encode the intendedMinConnsfallback fromMaxIdleConnectionsand the fact thatMinIdleConnsis only honored when explicitly configured. This aligns well with the implementation inpostgres.go.Also applies to: 174-229, 231-297
pkg/database/postgres/postgres.go (1)
33-39: Pool configuration defaults are correctly defined and documentedThe verification confirms all
_default*constants are properly defined inpkg/database/postgres/consts.gowith clear semantics:
- Each constant follows a consistent pattern:
0means "use pgxpool/pgx default," and explicit values override._defaultMaxConnLifetimeJitterdocuments its fallback behavior (20% ofMaxConnLifetime)._defaultMaxIdleConnectionsdeprecation is properly maintained for backward compatibility.- The constants are correctly wired to both read and write pool configs in the
newDBfunction.No issues found. The pool configuration is internally consistent and properly documented.
docs/setting-up/configuration.mdx (4)
97-106: Clear YAML example with inline documentation.The example properly demonstrates both new and deprecated fields, with helpful comments explaining defaults and pgxpool mappings. This makes backward compatibility explicit for users transitioning from old to new field names.
401-410: Well-organized structure documentation.The database configuration structure clearly distinguishes between new and deprecated fields, making the configuration hierarchy transparent and easy for users to navigate. Deprecation marking prevents user confusion.
425-434: Verify backward compatibility fallback logic matches implementation.The glossary clearly documents default values and behavioral nuances (e.g., "If not set, max_open_connections will be used for backward compatibility" on line 425, and the min_conns/max_idle_connections interaction on line 428). Please confirm these fallback behaviors align with the actual implementation in the config loading and options initialization code.
447-456: Complete ENV variable coverage with consistent naming.All new configuration fields have corresponding environment variable mappings with appropriate type annotations (int for connection counts, duration for timing values). Deprecated fields are retained for backward compatibility. The naming convention aligns with the existing PERMIFY_DATABASE_* pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/database/postgres/consts.go (1)
4-13: Defaults and comments look consistent with new pool behaviorThe new constants and “0 means use pgx/pgxpool default” semantics line up with how
newDBapplies them, and_defaultMaxIdleConnectionsis clearly marked as a back‑compat alias feedingMinConns. The jitter default comment also matches the later 20%‑of‑lifetime logic.If you ever need to support “explicitly no jitter”, you may want to distinguish between “unset” and “set to 0” rather than treating all zeros as “derive 20% from MaxConnectionLifetime, if any.”
internal/config/config.go (1)
360-369: Default database values preserve previous behavior while exposing new knobsThe defaults (
MaxConns: 0+MaxOpenConnections: 20,MaxIdleConnections: 1,MinConns: 0,MinIdleConns: 0) correctly preserve existing pool sizing when users don’t set the new fields, while still allowing the new*_connsand timing settings to be opted into. Comments describing howmax_connsandmax_idle_connectionsfall back into the new fields are consistent with the factory/postgres wiring.If you add more docs/examples later, it may be worth mirroring these exact phrases there so users see the same replacement/fallback rules everywhere.
internal/factories/database.go (1)
22-27: Extend DatabaseFactory config doc to list new timing optionsThe parameter list now covers the pool sizing fields and deprecations, but omits the new timing options (
HealthCheckPeriod,MaxConnLifetimeJitter,ConnectTimeout) that are also read fromconf.Databaseand passed via options.Consider adding bullets for the new duration fields here to keep the comment in sync with the actual configuration surface.
pkg/database/postgres/postgres.go (1)
127-146: Jitter, health check period, and connect timeout behavior are consistent with docs
MaxConnLifetimeJitter > 0is used as‑is; otherwise you compute~20%ofmaxConnectionLifeTime, matching the comments about default jitter derived from lifetime.HealthCheckPeriodandConnectTimeoutare only applied when >0, so zero keeps pgx/pgxpool defaults (1 minute health checks, no explicit connect timeout).If you ever need to allow “no jitter even when lifetime is set,” you might add an explicit sentinel (e.g., a negative duration) rather than treating all zeros as “derive 20% from lifetime.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/performance-test/values.yaml(1 hunks)internal/config/config.go(2 hunks)internal/factories/database.go(2 hunks)pkg/database/postgres/consts.go(1 hunks)pkg/database/postgres/options.go(2 hunks)pkg/database/postgres/postgres.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/database/postgres/options.go
- docs/performance-test/values.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
internal/config/config.go (1)
pkg/database/postgres/options.go (9)
MaxConns(17-21)MaxOpenConnections(12-14)MaxIdleConnections(26-30)MinConns(34-38)MinIdleConns(44-48)MaxConnectionIdleTime(51-55)HealthCheckPeriod(65-69)MaxConnLifetimeJitter(72-76)ConnectTimeout(79-83)
internal/factories/database.go (2)
pkg/database/postgres/options.go (14)
Option(8-8)MaxConnectionIdleTime(51-55)MaxConnectionLifeTime(58-62)WatchBufferSize(91-95)MaxDataPerWrite(85-89)MaxRetries(97-101)MinConns(34-38)MaxConns(17-21)MaxOpenConnections(12-14)MaxIdleConnections(26-30)MinIdleConns(44-48)HealthCheckPeriod(65-69)MaxConnLifetimeJitter(72-76)ConnectTimeout(79-83)pkg/database/postgres/postgres.go (2)
NewWithSeparateURIs(48-50)New(43-45)
pkg/database/postgres/postgres.go (1)
pkg/database/postgres/options.go (6)
MinConns(34-38)MinIdleConns(44-48)MaxConns(17-21)MaxConnLifetimeJitter(72-76)HealthCheckPeriod(65-69)ConnectTimeout(79-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Container for Vulnerabilities
- GitHub Check: Analyze (go)
- GitHub Check: Test with Coverage
- GitHub Check: Test with Coverage
🔇 Additional comments (4)
internal/config/config.go (1)
170-179: Database struct field semantics and deprecation story are coherentThe mapping and deprecation notes for
MaxConns,MaxOpenConnections,MaxIdleConnections,MinConns, andMinIdleConnsnow match the factory/newDB behavior (MaxConns primary; MaxOpenConnections fallback; MaxIdleConnections feeding MinConns only whenmin_connsis zero). The new timing knobs (HealthCheckPeriod,MaxConnLifetimeJitter,ConnectTimeout) are clearly documented as “0 = pgx/pgxpool defaults,” which aligns with their conditional application.internal/factories/database.go (1)
39-80: Option wiring and precedence for pool sizing and tuning are soundThe
optsconstruction cleanly centralizes all pool options:
- Core knobs (
MaxConnectionIdleTime,MaxConnectionLifeTime,WatchBufferSize,MaxDataPerWrite,MaxRetries) are always included.MinConnsis applied only when >0 and explicitly takes precedence over legacyMaxIdleConnections, which is kept for back‑compat viaPQDatabase.MaxIdleConnections.MaxConnsis primary; when zero,MaxOpenConnectionsis mapped intoMaxConns, preserving previous behavior.MinIdleConnsand the new timing options (HealthCheckPeriod,MaxConnLifetimeJitter,ConnectTimeout) are applied only when non‑zero, matching the “0 = use pgx/pgxpool defaults” semantics.Passing
opts...into bothNewWithSeparateURIsandNewensures consistent configuration across writer/read pools.pkg/database/postgres/postgres.go (2)
33-39: Struct fields and default initialization align with new config surfaceIntroducing
maxConns,minConns,minIdleConns,healthCheckPeriod,maxConnLifetimeJitter, andconnectTimeouton thePostgresstruct, with defaults taken from_default*constants, meshes cleanly with the new options and config fields. KeepingmaxIdleConnectionsonly as a deprecated back‑compat field is clear from the comment and safe given its limited use.Also applies to: 55-64
92-116: Min/Max pool sizing and legacymaxIdleConnectionsfallback look correctThe logic:
- Derives an effective
minConnsfrompg.minConns, falling back topg.maxIdleConnectionsonly whenminConns == 0and the legacy field is set.- Applies
MinConnsandMinIdleConnsto both write and read pools only when >0.- Sets
MaxConnsfor both pools only when >0, leaving pgxpool’s “unlimited” default otherwise.This matches the documented semantics in
config.Databaseand the factory: explicitMinConnsoverrides the legacy idle count, andMaxConnsis primary withMaxOpenConnectionsmapped into it upstream.
…on options
Summary by CodeRabbit
New Features
Documentation