feat: support PostgreSQL connection pool configuration - #5478
Conversation
✅ Deploy Preview for go-feature-flag-doc-preview canceled.
|
Greptile SummaryThis PR adds optional connection pool configuration (
Confidence Score: 5/5Safe to merge; all new fields are optional with zero-value backward compatibility and the pool cache correctly segregates configurations by URI+settings. The change is well-tested, backward-compatible, and the pool management logic is sound. The only gap is that validatePostgreSQLRetriever does not check the int32 upper bound for MaxConns/MinConns, meaning a theoretically out-of-range value passes IsValid() but fails at factory time — a minor validation inconsistency with no realistic production impact. cmdhelpers/retrieverconf/retriever_conf.go — the validatePostgreSQLRetriever function should mirror the int32 overflow check that already exists in createPostgreSQLRetriever. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5478 +/- ##
==========================================
+ Coverage 86.58% 87.04% +0.45%
==========================================
Files 160 160
Lines 6956 7257 +301
==========================================
+ Hits 6023 6317 +294
- Misses 694 697 +3
- Partials 239 243 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
| if c.ConnMaxLifetime != "" { | ||
| if _, errParse := time.ParseDuration(c.ConnMaxLifetime); errParse != nil { | ||
| return fmt.Errorf("invalid retriever configuration, "+ | ||
| "\"connMaxLifetime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse) | ||
| } | ||
| } | ||
| if c.ConnMaxIdleTime != "" { | ||
| if _, errParse := time.ParseDuration(c.ConnMaxIdleTime); errParse != nil { | ||
| return fmt.Errorf("invalid retriever configuration, "+ | ||
| "\"connMaxIdleTime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse) | ||
| } | ||
| } |
There was a problem hiding this comment.
time.ParseDuration accepts negative values like "-1h" without error, so the validation here passes. The value then flows into PoolConfig.ConnMaxLifetime or PoolConfig.ConnMaxIdleTime. In newPool, both fields have a > 0 guard — meaning a negative duration is silently discarded and the driver default is kept instead. The user receives no error and no indication that their setting was not applied.
| if c.ConnMaxLifetime != "" { | |
| if _, errParse := time.ParseDuration(c.ConnMaxLifetime); errParse != nil { | |
| return fmt.Errorf("invalid retriever configuration, "+ | |
| "\"connMaxLifetime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse) | |
| } | |
| } | |
| if c.ConnMaxIdleTime != "" { | |
| if _, errParse := time.ParseDuration(c.ConnMaxIdleTime); errParse != nil { | |
| return fmt.Errorf("invalid retriever configuration, "+ | |
| "\"connMaxIdleTime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse) | |
| } | |
| } | |
| if c.ConnMaxLifetime != "" { | |
| d, errParse := time.ParseDuration(c.ConnMaxLifetime) | |
| if errParse != nil { | |
| return fmt.Errorf("invalid retriever configuration, "+ | |
| "\"connMaxLifetime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse) | |
| } | |
| if d <= 0 { | |
| return fmt.Errorf("invalid retriever configuration, "+ | |
| "\"connMaxLifetime\" must be a positive duration for kind \"%s\"", c.Kind) | |
| } | |
| } | |
| if c.ConnMaxIdleTime != "" { | |
| d, errParse := time.ParseDuration(c.ConnMaxIdleTime) | |
| if errParse != nil { | |
| return fmt.Errorf("invalid retriever configuration, "+ | |
| "\"connMaxIdleTime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse) | |
| } | |
| if d <= 0 { | |
| return fmt.Errorf("invalid retriever configuration, "+ | |
| "\"connMaxIdleTime\" must be a positive duration for kind \"%s\"", c.Kind) | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmdhelpers/retrieverconf/retriever_conf.go
Line: 137-148
Comment:
**Negative duration strings bypass validation but are silently ignored at runtime**
`time.ParseDuration` accepts negative values like `"-1h"` without error, so the validation here passes. The value then flows into `PoolConfig.ConnMaxLifetime` or `PoolConfig.ConnMaxIdleTime`. In `newPool`, both fields have a `> 0` guard — meaning a negative duration is silently discarded and the driver default is kept instead. The user receives no error and no indication that their setting was not applied.
```suggestion
if c.ConnMaxLifetime != "" {
d, errParse := time.ParseDuration(c.ConnMaxLifetime)
if errParse != nil {
return fmt.Errorf("invalid retriever configuration, "+
"\"connMaxLifetime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse)
}
if d <= 0 {
return fmt.Errorf("invalid retriever configuration, "+
"\"connMaxLifetime\" must be a positive duration for kind \"%s\"", c.Kind)
}
}
if c.ConnMaxIdleTime != "" {
d, errParse := time.ParseDuration(c.ConnMaxIdleTime)
if errParse != nil {
return fmt.Errorf("invalid retriever configuration, "+
"\"connMaxIdleTime\" is not a valid duration for kind \"%s\": %w", c.Kind, errParse)
}
if d <= 0 {
return fmt.Errorf("invalid retriever configuration, "+
"\"connMaxIdleTime\" must be a positive duration for kind \"%s\"", c.Kind)
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Address CI failures on thomaspoignant#5478: - gosec G115: bounds-check MaxOpenConns/MaxIdleConns before narrowing int to int32, returning an error for out-of-range values. - check-mode golden tests: add json:",omitempty" to the optional pool fields so they no longer serialize zero values into every retriever's config output.
|
Fixed both CI failures:
|
thomaspoignant
left a comment
thomaspoignant
left a comment
There was a problem hiding this comment.
Hey @mvanhorn I've done a first path of reviews (not complete yet), but I have addressed some small issues here and there.
I will perform a more in depth review as soon as possible.
| if poolCfg.MaxIdleConns > 0 { | ||
| config.MinConns = poolCfg.MaxIdleConns | ||
| } |
There was a problem hiding this comment.
🔨 issue: why do you put MaxIdleConns inside the MinConns pgxpool config here ?
Should it me
config.MaxIdleConns = poolCfg.MaxIdleConns| config.MaxConnIdleTime = poolCfg.ConnMaxIdleTime | ||
| } | ||
|
|
||
| return pgxpool.NewWithConfig(ctx, config) |
There was a problem hiding this comment.
It would be great here to have a check that we don't have MinConns > MaxConns
Something like this would work:
| return pgxpool.NewWithConfig(ctx, config) | |
| if config.MinConns > config.MaxConns { | |
| return nil, fmt.Errorf( | |
| "invalid pool configuration: maxIdleConns (%d) must not exceed maxOp | |
| enConns (%d)", | |
| config.MinConns, config.MaxConns) | |
| } | |
| return pgxpool.NewWithConfig(ctx, config) |
| if poolCfg.MaxIdleConns > 0 { | ||
| config.MinConns = poolCfg.MaxIdleConns | ||
| } | ||
| if poolCfg.ConnMaxLifetime > 0 { |
There was a problem hiding this comment.
🤓 nitpick: If we want to test fully.
| if poolCfg.ConnMaxLifetime > 0 { | |
| if c.MaxOpenConns < 0 || c.MaxOpenConns > math.MaxInt32 { |
| if poolCfg.ConnMaxLifetime > 0 { | ||
| config.MaxConnLifetime = poolCfg.ConnMaxLifetime | ||
| } | ||
| if poolCfg.ConnMaxIdleTime > 0 { |
There was a problem hiding this comment.
🤓 nitpick: very very nit pick
| if poolCfg.ConnMaxIdleTime > 0 { | |
| if poolCfg.ConnMaxIdleTime > 0 || c.ConnMaxIdleTime > math.MaxInt32 { |
| if poolCfg.MaxOpenConns > 0 { | ||
| config.MaxConns = poolCfg.MaxOpenConns |
There was a problem hiding this comment.
🔨 issue: why do you put MaxOpenConns inside the MaxConns pgxpool config here ?
Should it be?
config.MaxOpenConns = poolCfg.MaxOpenConnsPer review: the config borrowed database/sql names (MaxOpenConns/MaxIdleConns) but MaxIdleConns (a maximum) was mapped onto pgxpool's MinConns (a minimum floor), which is misleading. Renamed the pool config fields and yaml/json keys to pgxpool's own model (maxConns/minConns) so the mapping is 1:1 and self-documenting. Adds a MinConns > MaxConns validation and corrects the int32 bounds checks (< 0 || > MaxInt32) for the narrowing conversions.
|
Good catch on the mapping - you're right that borrowing database/sql's MaxOpenConns/MaxIdleConns names was misleading, since MaxIdleConns (a max) was landing on pgxpool's MinConns (a minimum floor). Renamed the pool config to pgxpool's own model: maxConns/minConns, mapped 1:1 (config.MaxConns = poolCfg.MaxConns, config.MinConns = poolCfg.MinConns). Updated the yaml/json keys, docs, and tests to match. Also applied your suggestions: a MinConns > MaxConns validation (both at the retriever config layer and in newPool), and fixed the int32 bounds checks to < 0 || > math.MaxInt32 for both fields. Verified: the pool config tests pass locally (IsZero, AppliesConfig, the MinConns>MaxConns guard, partial config, invalid URI) and go vet is clean. The one failing test, TestGetPool_MultipleURIsAndReuse, spins up a real Postgres via testcontainers and needs Docker running, which I don't have here - it's unaffected by the rename. |
|
The Govulncheck failure is GO-2026-5856, a Go stdlib (crypto/tls) vulnerability fixed in go1.26.5 - it's unrelated to this PR's diff and will affect any branch on go1.26.4. Bumped the go directive to 1.26.5 and re-ran make's vuln-check target locally: 0 affecting vulnerabilities now. If you'd rather take the bump on main separately, happy to drop the commit from this PR. |
|
2 participants
Description
The PostgreSQL retriever previously exposed only
uri,table, and customcolumn names, leaving the connection pool entirely at the driver defaults.
Production workloads on managed Postgres need control over pool sizing and
connection lifetimes for performance and resource safety.
This PR adds optional, per-retriever connection pool settings, following the
design the maintainer selected in #4835:
maxOpenConns-> pgxpoolMaxConnsmaxIdleConns-> pgxpoolMinConns(the pool floor; pgxpool has no separateidle ceiling, idle connections are bounded by
MaxConns)connMaxLifetime-> pgxpoolMaxConnLifetimeconnMaxIdleTime-> pgxpoolMaxConnIdleTimeHow it works:
PoolConfigis threaded through thepgxpoolpool builder at theexisting
GetPool/poolMapseam inpostgres.go. When no pool block isconfigured, the pool is built exactly as before (
pgxpool.New), so existingconfigurations are byte-for-byte unaffected. When a pool block is present, the
URI is parsed with
pgxpool.ParseConfigand only the explicitly set fields areoverridden.
poolMapis now keyed by URI and the pool config, so tworetrievers that share a URI but request different pool settings no longer
silently reuse the first pool created.
RetrieverConf(relay proxy / config files)with validation in
validatePostgreSQLRetriever()(negative values,maxIdleConnsgreater thanmaxOpenConns, and unparseable durations arerejected with a clear error) and wired through
createPostgreSQLRetriever.How to test: see the new unit tests in
retriever/postgresqlretriever/postgres_pool_test.go,cmdhelpers/retrieverconf/retriever_conf_test.go, andcmdhelpers/retrieverconf/init/retriever_init_test.go. Building a pool does notopen a connection, so the pool-config tests run without a live database; the
existing testcontainers-backed pool test continues to cover end-to-end behavior.
No breaking change to configuration behavior: every new field is optional and
defaults preserve the current behavior.
Closes issue(s)
Resolve #4835
Checklist
README.mdand/website/docs)Fixes #4835