fix(db): Honour an explicitly configured min_conns of 0 - #565
Conversation
POSTGRESQL_MIN_CONNS / HARBOR_DATABASE_MIN_CONNS could not be set to 0. Both the config layer and dbpool treated 0 as "unset" and substituted DefaultMinConns (2), so there was no way to run pgxpool without a warm connection floor. That floor is multiplied across every tenant sharing one PostgreSQL server in dense multi-tenant deployments. 0 is pgxpool's own defaultMinConns, not a degenerate value: the pool keeps no warm connections, drains to empty when idle, and opens on demand at acquire time. MaxConns still governs the ceiling, so burst behaviour is unchanged. The cost is connect latency on the first query after an idle gap. DefaultMinConns = 2 was itself inherited from the removed POSTGRESQL_MAX_IDLE_CONNS in 612a3c5 (#118) — but that was a cap on idle connections, whereas MinConns is a floor. The number crossed an inverted semantic. The default stays 2; it is now overridable. - models.PostGreSQL.MinConns becomes *int32: nil = unset, 0 = configured. - dbpool.applyPoolConfig honours an explicit 0 and returns an error for a negative value instead of silently falling back; New propagates it, so a bad value fails startup rather than quietly changing pool behaviour. - ConfigureValue.GetOptionalInt32 returns nil for a key with no metadata, so a missing key is not read back as a configured 0 (CfgManager.Get yields an empty ConfigureValue whose GetInt is 0). - The exporter uses viper.IsSet, which distinguishes an unset env var from HARBOR_DATABASE_MIN_CONNS=0; viper.GetInt collapses both to 0. Closes #564 Signed-off-by: Vadim Bauer <vb@container-registry.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
|
|
This change may need patch-release backports. Comment with one of these commands to open a cherry-pick PR:
|
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change makes PostgreSQL ChangesPostgreSQL minimum-connections configuration
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cmd/exporter/main_test.go`:
- Around line 43-50: Update the subtest setup around viper.Reset and the
HARBOR_DATABASE_MIN_CONNS environment variable to explicitly isolate that
variable before each case. Ensure the set: false path cannot inherit a value
from the test runner, while preserving the existing tt.set behavior for cases
that provide tt.env.
In `@src/lib/config/metadata/value.go`:
- Around line 91-92: Validate the parsed MinConns value against the int32 range
before narrowing in the value conversion logic around GetInt, returning a
startup error for values outside math.MinInt32 through math.MaxInt32; update
both src/lib/config/metadata/value.go:91-92 and src/cmd/exporter/main.go:135-136
as applicable, and add boundary coverage for math.MaxInt32 + 1 and math.MinInt32
- 1.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00482ced-a870-4436-9822-1bb3b6e2af19
📒 Files selected for processing (11)
src/cmd/exporter/main.gosrc/cmd/exporter/main_test.gosrc/common/models/database.gosrc/lib/config/metadata/value.gosrc/lib/config/metadata/value_test.gosrc/lib/config/systemconfig.gosrc/lib/dbpool/pool.gosrc/lib/dbpool/pool_integration_test.gosrc/lib/dbpool/pool_test.gosrc/pkg/config/inmemory/manager_test.gosrc/pkg/config/manager.go
There was a problem hiding this comment.
Pull request overview
Fixes the configuration path so database.minConns can be explicitly set to 0 (pgxpool’s default) instead of being treated as “unset” and coerced to DefaultMinConns (2). This improves correctness for multi-tenant / low-traffic deployments where a warm-connection floor is undesirable, while also making invalid negative values fail fast.
Changes:
- Represent
models.PostGreSQL.MinConnsas*int32sonilmeans “unset” and0is an explicit, valid value. - Update dbpool configuration application to honor explicit
0and reject negative values with a startup error. - Add config/exporter handling + tests to preserve explicit
0through config read/export paths.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pkg/config/manager.go | Uses GetOptionalInt32() so the config layer can preserve an explicit 0 vs “unset”. |
| src/pkg/config/inmemory/manager_test.go | Adds an end-to-end-ish unit test asserting explicit 0 survives config store → GetDatabaseCfg(). |
| src/lib/dbpool/pool.go | Makes applyPoolConfig return an error; honors *int32 minConns including explicit 0, rejects negatives. |
| src/lib/dbpool/pool_test.go | Updates tests for pointer-based MinConns and adds explicit coverage for 0, nil, and negative values. |
| src/lib/dbpool/pool_integration_test.go | Updates integration tests for pointer-based MinConns and fixes deref usage. |
| src/lib/config/systemconfig.go | Uses GetOptionalInt32() for system config database min conns. |
| src/lib/config/metadata/value.go | Adds ConfigureValue.GetOptionalInt32() to distinguish missing-metadata from configured 0. |
| src/lib/config/metadata/value_test.go | Adds tests for GetOptionalInt32() behavior (but currently has an order-dependence issue). |
| src/common/models/database.go | Changes MinConns to *int32 with omitempty semantics. |
| src/cmd/exporter/main.go | Introduces getMinConns() to return nil unless the env var is set, preserving explicit 0. |
| src/cmd/exporter/main_test.go | Adds tests asserting unset/empty/0/5 behavior for exporter getMinConns(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review follow-ups on #565: - GetOptionalInt32 and the exporter's getMinConns validated only after narrowing to int32, but both parse paths admit wider values (parseInt via its float fallback, viper via cast.ToInt). 1<<32 wrapped to an explicit 0 — the opposite of the configured intent, and invisible to the negativity check. Out-of-range now logs and reads as unset. - TestGetMinConns isolates HARBOR_DATABASE_MIN_CONNS from the ambient environment; viper.Reset does not clear the process env, so a CI runner exporting the variable broke the unset case. - TestConfigureValue_GetOptionalInt32 pins the real metadata ConfigList; neighbouring tests swap in testingMetaDataArray without restoring it, which lacks postgresql_min_conns, making the test order-dependent. Signed-off-by: Vadim Bauer <vb@container-registry.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Preview images for this PR are available in
Verify a preview image: Verify SBOM attestation: |
| if v < math.MinInt32 || v > math.MaxInt32 { | ||
| log.Errorf("GetOptionalInt32 failed: value %d of %s exceeds int32 range, treating as unset", v, c.Name) | ||
| return nil |
| if v < math.MinInt32 || v > math.MaxInt32 { | ||
| log.Warningf("database.min_conns %d exceeds int32 range, using the pool default", v) | ||
| return nil |
Summary
database.minConnscould not be set to0. The Go config layer treated0as "unset" and substitutedDefaultMinConns(2), so there was no way to run pgxpool without a warm-connection floor. In dense multi-tenant deployments that floor is multiplied across every tenant sharing one PostgreSQL server.0is not a degenerate value — it is pgxpool's owndefaultMinConns. The pool keeps no warm connections, drains to empty when idle, and opens on demand at acquire time.MaxConnsstill governs the ceiling, so burst behaviour is unchanged. The cost is connection setup latency (TCP + TLS + auth + backend fork) on the first query after an idle gap.Why the default was 2
Worth recording, since it looks deliberate and is not. PR #118 (
612a3c5f7) replaced one metadata line with another in the same hunk:The
database/sqlknob was a cap on idle connections (keep at most 2 idle); the pgxpool knob is a floor (keep at least 2 warm). The number crossed an inverted semantic. Theif x > 0 { } else { default }guard is the "0 means not set" idiom copied fromMaxOpenConnsdirectly above it, where 0 genuinely does mean unset for pgxpool.The default stays 2. It is now overridable.
Changes
models.PostGreSQL.MinConnsbecomes*int32— nil is "unset", 0 is a configured value.dbpool.applyPoolConfighonours an explicit 0 and returns an error for a negative value instead of silently falling back.Newpropagates it, so a bad value fails startup rather than quietly changing pool behaviour.ConfigureValue.GetOptionalInt32returns nil when the key carries no metadata, so a missing key is not read back as a configured 0 (CfgManager.Gethands back an emptyConfigureValuewhoseGetIntis 0).viper.IsSet, which distinguishes an unset env var fromHARBOR_DATABASE_MIN_CONNS=0;viper.GetIntcollapses both to 0.Related Issues
Closes #564
Chart half is on #56 (
fix(chart): Allow database.minConns 0, let exporter.config win over chart env). Both halves are needed end to end: the chart used to coerce0to2in its templates independently of this.Type of Change
fix:)Testing
Acceptance criteria from the issue, each asserted directly:
MinConns: ptr(0)→pgxpool.Config.MinConns == 0TestApplyPoolConfig_ExplicitZeroMinConnsHonoredMinConns: nil→ 2TestApplyPoolConfig_NilMinConnsUsesDefaultTestApplyPoolConfig_NegativeMinConnsRejectedPOSTGRESQL_MIN_CONNS=0survives the whole core pathTestGetDatabaseCfg_MinConns(config store →GetDatabaseCfg→models.PostGreSQL)HARBOR_DATABASE_MIN_CONNSunset /""/0/5TestGetMinConnsTestConfigureValue_GetOptionalInt32Note for reviewers: five
db-tagged integration cases setcfg.MinConns = 0with a// don't pre-createcomment, which the old coercion silently turned into 2. They now get what they asked for.go test -tags db ./lib/dbpool/...needs a live PostgreSQL and has not been run locally.Checklist
git commit -s)