feat: configurable, reused DB connection pool - #434
Open
jabbrwcky wants to merge 8 commits into
Open
Conversation
Every DB client previously opened a new *sql.DB via sql.Open on every query and closed it immediately, so the provider held an effectively unbounded number of short-lived connections against each server. Add a process-wide connection-pool cache keyed by driver and DSN: managed resources that target the same server (same driver + DSN) now share a single bounded *sql.DB. Config carries MaxOpenConns / MaxIdleConns / ConnMaxLifetime / ConnMaxIdleTime; pools that go unused are evicted so a credential rotation (which changes the DSN) does not leak pools. This package has no callers yet; it is wired into the DB clients and made configurable via ProviderConfig in the following commits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
Exec/Query/Scan (and postgresql's ExecTx) now fetch a connection from pool.Get(driver, dsn, cfg) instead of calling sql.Open and closing it after every call. New(...) for all three clients gains a trailing pool.Config parameter so callers can tune the shared pool. No caller passes a non-zero-value pool.Config yet; that lands with the ProviderConfig API and controller wiring in the following commits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
Add an optional ConnectionPool field (MaxOpenConnections, MaxIdleConnections, MaxConnLifetime, MaxConnIdleTime) plus a nil-safe ToPoolConfig() method to every ProviderConfigSpec / ClusterProviderConfigSpec, for all three databases and both cluster and namespaced scopes. Omitted fields, or an omitted connectionPool block entirely, default to pool.Default. Regenerate zz_generated.deepcopy.go and the CRD manifests under package/crds via `make generate`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
Every connector's newDB/newClient field and every Connect implementation now computes a pool.Config from the resource's ProviderConfig (cluster controllers via pc.Spec.ConnectionPool, namespaced controllers via the provider.GetProviderConfig helper's ProviderInfo.PoolConfig) and passes it as the last argument to the client constructor. The 3 namespaced provider.go helpers (mysql/postgresql/mssql) gain a PoolConfig field on ProviderInfo, populated from ConnectionPool.ToPoolConfig() for both the ProviderConfig and ClusterProviderConfig kinds. Test mocks are updated to match the new constructor signatures. With this commit the connection-pool feature is fully wired end to end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
Add docs/connection-pool.md covering the connectionPool fields, defaults (maxOpenConnections=10, maxIdleConnections=5, maxConnLifetime=1h, maxConnIdleTime=10m), and pool-sharing/eviction behavior. Add a connectionPool block to the cluster ProviderConfig example for each database. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
The cache test opener called sql.Open with an unregistered driver name, which returns (nil, err) on current Go. The stub ignored the error and cached a nil *sql.DB, so the cleanup's db.Close() nil-panicked in CI (SIGSEGV in TestGetReusesSamePool). Hand the cache a real, non-nil, closeable *sql.DB via sql.OpenDB with a fake driver.Connector — no registered driver needed and the tests never query. Also nil-guard the cleanup Close defensively. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
ToPoolConfig() (6 apis packages), pool.Get's error path, Config.apply, and
the 3 namespaced GetProviderConfig helpers were new logic introduced by the
connection-pool feature with 0% coverage anywhere in the repo. Add targeted
unit tests for each:
- ToPoolConfig(): table-driven, nil receiver / per-field defaulting / all-set,
in all 6 apis/{cluster,namespaced}/{mysql,postgresql,mssql}/v1alpha1
packages that define the (duplicated) ConnectionPool type.
- pool.Get: error propagation when openDB fails (nothing gets cached).
- Config.apply: verifies MaxOpenConnections via sql.DBStats (the only field
database/sql exposes a public getter for); the other three setters are
smoke-tested since they have no equivalent public getter.
- GetProviderConfig (namespaced mysql/postgresql/mssql provider helpers):
both ProviderConfig/ClusterProviderConfig branches, error paths, and that
a connectionPool override reaches ProviderInfo.PoolConfig, using the
existing test.MockClient pattern already used by this repo's reconciler
tests.
Deliberately out of scope: DB client Exec/Query/Scan and reconciler Connect
success paths remain 0%-covered, but that predates this branch (confirmed
unchanged vs master) and would need a fake-driver/sqlmock or fake-kube-client
investment disproportionate to this change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
2 tasks
jabbrwcky
added a commit
to jabbrwcky/provider-sql
that referenced
this pull request
Aug 18, 2026
…ane-contrib#434 The "bounded shared connection pool" follow-up listed here landed as its own change; point at docs/connection-pool.md instead of leaving it as an open TODO. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks
…ed examples The connectionPool config was already supported on the namespaced ProviderConfig/ClusterProviderConfig APIs but the namespaced example manifests didn't demonstrate it, and the doc didn't call out the cluster vs namespaced API group split. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
jabbrwcky
force-pushed
the
feat/configurable-connection-pool
branch
from
August 18, 2026 12:50
b77e1aa to
d74854c
Compare
jabbrwcky
added a commit
to jabbrwcky/provider-sql
that referenced
this pull request
Aug 19, 2026
…ane-contrib#434 The "bounded shared connection pool" follow-up listed here landed as its own change; point at docs/connection-pool.md instead of leaving it as an open TODO. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jens Hausherr <jens.hausherr@pflege.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Every DB client previously opened a new
*sql.DBviasql.Openon every query and closed it immediately, so the provider held an effectively unbounded number of short-lived connections against each server.This PR introduces a process-wide, DSN-keyed connection-pool cache (
pkg/clients/pool): managed resources targeting the same server (driver + DSN) now share a single bounded*sql.DB. Pools applyMaxOpenConns/MaxIdleConns/ConnMaxLifetime/ConnMaxIdleTimeand are evicted after going idle, so a credential rotation (new DSN) does not leak pools.The pool is tunable per
ProviderConfig/ClusterProviderConfigvia a new optionalconnectionPoolblock, for all three databases (MySQL, PostgreSQL, MSSQL), cluster- and namespaced-scoped. Omitted fields use defaults:maxOpenConnections=10,maxIdleConnections=5,maxConnLifetime=1h,maxConnIdleTime=10m.Notable design points for reviewers:
xsql.DBinterface intentionally gains noClose(): the cache owns pool lifecycle, so the per-controllerDisconnectmethods stay no-ops.(driver, DSN); the config that first creates a pool for a DSN wins (in practice a DSN maps to one ProviderConfig).connectionPool, so existing ProviderConfigs keep working unchanged.See
docs/connection-pool.mdfor details.Fixes #195
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
makeandmake reviewablepass (generate is a no-op diff, lint and fullgo test ./...are green).🤖 Generated with Claude Code