diff --git a/database/connector.go b/database/connector.go index fe76b70..8084bc4 100644 --- a/database/connector.go +++ b/database/connector.go @@ -50,7 +50,7 @@ func createConnector(configFile string) CreateConnectorFunc { if err != nil { return nil, err } - defer dbFile.Close() + defer func() { _ = dbFile.Close() }() dec := yaml.NewDecoder(dbFile) cfg := Config{} diff --git a/database/integration_test.go b/database/integration_test.go index 1c68228..4f881c4 100644 --- a/database/integration_test.go +++ b/database/integration_test.go @@ -7,9 +7,7 @@ import ( ) // Mock types for testing SQLC integration patterns -type mockQueries struct { - db DBTX -} +type mockQueries struct{} type mockQueriesTx struct { *mockQueries @@ -58,7 +56,7 @@ func TestSQLCIntegration(t *testing.T) { // Verify our DB interface is compatible with what SQLC expects var dbInterface DB[*mockQueriesTx] - var mockDB *mockQueries = &mockQueries{} + mockDB := &mockQueries{} dbInterface = mockDB // Test that our transaction helper can work with this pattern diff --git a/database/metrics.go b/database/metrics.go index 97dba4f..db4bfce 100644 --- a/database/metrics.go +++ b/database/metrics.go @@ -3,10 +3,10 @@ package database import ( "context" "database/sql" - "fmt" "time" "github.com/prometheus/client_golang/prometheus" + "go.ntppool.org/common/logger" ) // DatabaseMetrics holds the Prometheus metrics for database connection pool monitoring @@ -16,6 +16,10 @@ type DatabaseMetrics struct { ConnectionsInUse prometheus.Gauge ConnectionsWaitCount prometheus.Counter ConnectionsWaitDuration prometheus.Histogram + + // Track last values for delta calculation (cumulative stats from sql.DBStats) + lastWaitCount int64 + lastWaitDuration time.Duration } // NewDatabaseMetrics creates a new set of database metrics and registers them @@ -67,26 +71,44 @@ func monitorConnectionPool(ctx context.Context, db *sql.DB, registerer prometheu ticker := time.NewTicker(30 * time.Second) defer ticker.Stop() + log := logger.FromContext(ctx) + for { select { case <-ctx.Done(): + log.InfoContext(ctx, "database connection pool monitor stopped") return case <-ticker.C: stats := db.Stats() + // Update gauge metrics (current state) metrics.ConnectionsOpen.Set(float64(stats.OpenConnections)) metrics.ConnectionsIdle.Set(float64(stats.Idle)) metrics.ConnectionsInUse.Set(float64(stats.InUse)) - metrics.ConnectionsWaitCount.Add(float64(stats.WaitCount)) - if stats.WaitDuration > 0 { - metrics.ConnectionsWaitDuration.Observe(stats.WaitDuration.Seconds()) + // Update counter with delta (WaitCount is cumulative in sql.DBStats) + waitCountDelta := stats.WaitCount - metrics.lastWaitCount + if waitCountDelta > 0 { + metrics.ConnectionsWaitCount.Add(float64(waitCountDelta)) + metrics.lastWaitCount = stats.WaitCount + } + + // Update histogram with delta (WaitDuration is cumulative in sql.DBStats) + waitDurationDelta := stats.WaitDuration - metrics.lastWaitDuration + if waitDurationDelta > 0 { + metrics.ConnectionsWaitDuration.Observe(waitDurationDelta.Seconds()) + metrics.lastWaitDuration = stats.WaitDuration } // Log connection pool stats for high usage or waiting - if stats.OpenConnections > 20 || stats.WaitCount > 0 { - fmt.Printf("Connection pool stats: open=%d idle=%d in_use=%d wait_count=%d wait_duration=%s\n", - stats.OpenConnections, stats.Idle, stats.InUse, stats.WaitCount, stats.WaitDuration) + if stats.OpenConnections > 20 || waitCountDelta > 0 { + log.WarnContext(ctx, "high database connection usage", + "open", stats.OpenConnections, + "idle", stats.Idle, + "in_use", stats.InUse, + "wait_count", stats.WaitCount, + "wait_duration", stats.WaitDuration, + ) } } } diff --git a/database/pgdb/config.go b/database/pgdb/config.go index 98f4184..426152f 100644 --- a/database/pgdb/config.go +++ b/database/pgdb/config.go @@ -27,15 +27,10 @@ func CreatePoolConfig(cfg *database.PostgresConfig) (*pgxpool.Config, error) { "require": true, "verify-ca": true, "verify-full": true, } if cfg.SSLMode != "" && !validSSLModes[cfg.SSLMode] { - return nil, fmt.Errorf("postgres: invalid sslmode: %s", cfg.SSLMode) - } - - // Set defaults - host := cfg.Host - if host == "" { - host = "localhost" + return nil, fmt.Errorf("postgres: invalid sslmode: %s (valid: disable, allow, prefer, require, verify-ca, verify-full)", cfg.SSLMode) } + // Apply defaults for optional fields (host is validated as required above) port := cfg.Port if port == 0 { port = 5432 @@ -49,7 +44,7 @@ func CreatePoolConfig(cfg *database.PostgresConfig) (*pgxpool.Config, error) { // Build connection string WITHOUT password // We'll set the password separately in the config connString := fmt.Sprintf("host=%s port=%d user=%s dbname=%s sslmode=%s", - host, port, cfg.User, cfg.Name, sslmode) + cfg.Host, port, cfg.User, cfg.Name, sslmode) // Parse the connection string poolConfig, err := pgxpool.ParseConfig(connString) diff --git a/database/pgdb/pool.go b/database/pgdb/pool.go index 7a7b4e9..e93dc14 100644 --- a/database/pgdb/pool.go +++ b/database/pgdb/pool.go @@ -2,12 +2,14 @@ package pgdb import ( "context" + "errors" "fmt" "os" "time" "github.com/jackc/pgx/v5/pgxpool" "go.ntppool.org/common/database" + "go.ntppool.org/common/logger" "gopkg.in/yaml.v3" ) @@ -61,6 +63,19 @@ func DefaultPoolOptions() PoolOptions { // can be specified in the URI query string and PoolOptions are ignored. // When using config files, PoolOptions are applied. func OpenPool(ctx context.Context, options PoolOptions) (*pgxpool.Pool, error) { + log := logger.FromContext(ctx) + + // Validate PoolOptions + if options.MaxConns <= 0 { + return nil, fmt.Errorf("pgdb: MaxConns must be positive, got: %d", options.MaxConns) + } + if options.MinConns < 0 { + return nil, fmt.Errorf("pgdb: MinConns must be non-negative, got: %d", options.MinConns) + } + if options.MinConns > options.MaxConns { + return nil, fmt.Errorf("pgdb: MinConns (%d) cannot exceed MaxConns (%d)", options.MinConns, options.MaxConns) + } + var poolConfig *pgxpool.Config var err error @@ -70,7 +85,15 @@ func OpenPool(ctx context.Context, options PoolOptions) (*pgxpool.Pool, error) { if err != nil { return nil, fmt.Errorf("failed to parse DATABASE_URI: %w", err) } - // Pool settings from URI are used; PoolOptions ignored + + // Log when PoolOptions differ from defaults (they will be ignored) + defaults := DefaultPoolOptions() + if options.MaxConns != defaults.MaxConns || options.MinConns != defaults.MinConns { + log.WarnContext(ctx, "DATABASE_URI is set; PoolOptions are ignored (use URI query parameters for pool settings)", + "ignored_max_conns", options.MaxConns, + "ignored_min_conns", options.MinConns, + ) + } } else { // Fall back to config file approach pgCfg, err := findAndParseConfig(options.ConfigFiles) @@ -99,7 +122,7 @@ func OpenPool(ctx context.Context, options PoolOptions) (*pgxpool.Pool, error) { // Test the connection if err := pool.Ping(ctx); err != nil { - pool.Close() + pool.Close() // pgxpool.Pool.Close() doesn't return an error return nil, fmt.Errorf("failed to ping database: %w", err) } @@ -116,35 +139,33 @@ func OpenPoolWithConfigFile(ctx context.Context, configFile string) (*pgxpool.Po // findAndParseConfig searches for and parses the first existing config file func findAndParseConfig(configFiles []string) (*database.PostgresConfig, error) { - var firstErr error + var errs []error + var triedFiles []string for _, configFile := range configFiles { if configFile == "" { continue } + triedFiles = append(triedFiles, configFile) // Check if file exists if _, err := os.Stat(configFile); err != nil { - if firstErr == nil { - firstErr = err - } + errs = append(errs, fmt.Errorf("%s: %w", configFile, err)) continue } // Try to read and parse the file pgCfg, err := parseConfigFile(configFile) if err != nil { - if firstErr == nil { - firstErr = err - } + errs = append(errs, fmt.Errorf("%s: %w", configFile, err)) continue } return pgCfg, nil } - if firstErr != nil { - return nil, fmt.Errorf("no config file found: %w", firstErr) + if len(errs) > 0 { + return nil, fmt.Errorf("no valid config file found (tried: %v): %w", triedFiles, errors.Join(errs...)) } return nil, fmt.Errorf("no valid config files provided") } @@ -155,7 +176,7 @@ func parseConfigFile(configFile string) (*database.PostgresConfig, error) { if err != nil { return nil, fmt.Errorf("failed to open config file: %w", err) } - defer file.Close() + defer func() { _ = file.Close() }() dec := yaml.NewDecoder(file) cfg := database.Config{} diff --git a/database/pool.go b/database/pool.go index 7e649ae..396661e 100644 --- a/database/pool.go +++ b/database/pool.go @@ -51,9 +51,9 @@ func OpenDBWithConfigFile(ctx context.Context, configFile string) (*sql.DB, erro // OpenDBMonitor opens a database connection with monitor-specific defaults // This is a convenience function for Monitor package compatibility -func OpenDBMonitor() (*sql.DB, error) { +func OpenDBMonitor(ctx context.Context) (*sql.DB, error) { options := MonitorConfigOptions() - return OpenDB(context.Background(), options) + return OpenDB(ctx, options) } // findConfigFile searches for the first existing config file from the list