From 0b47a5963c703b6b4b9f2f740811d31173a270ee Mon Sep 17 00:00:00 2001 From: Aaron Tatar Date: Mon, 7 Apr 2025 09:20:01 +0200 Subject: [PATCH 1/2] Add in provider options for DB Connection Pools (#3) * Add in provider options for DB Connection Pools The defaults maintain the current configuration, but this allows users to tune db connections for individual needs. In particular, even with the latest go libraries, we often encounter "cannot allocate memory" errors when creating yet another DB connection. We have a large number of resources across multiple DBs, so this happens frequently. For our uses, enabling connection pooling is going to save us a lot of headaches and the edge case disabling the connection pooling is guarding against unlikely to be an issue. * Use `any` and add some documentation for connection timing (cherry picked from commit c546ea4be96a9450fbe2299ef80b95e16dd30dea) --- postgresql/config.go | 8 +++++- postgresql/provider.go | 55 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/postgresql/config.go b/postgresql/config.go index c2f1410c..eac08403 100644 --- a/postgresql/config.go +++ b/postgresql/config.go @@ -8,6 +8,7 @@ import ( "strconv" "strings" "sync" + "time" "unicode" "github.com/blang/semver" @@ -179,6 +180,9 @@ type Config struct { Timeout int ConnectTimeoutSec int MaxConns int + MaxIdleConns int + ConnMaxIdleTime time.Duration + ConnMaxLifetime time.Duration ExpectedVersion semver.Version SSLClientCert *ClientCertificateConfig SSLRootCertPath string @@ -308,8 +312,10 @@ func (c *Client) Connect() (*DBConnection, error) { // We don't want to retain connection // So when we connect on a specific database which might be managed by terraform, // we don't keep opened connection in case of the db has to be dropped in the plan. - db.SetMaxIdleConns(0) + db.SetMaxIdleConns(c.config.MaxIdleConns) db.SetMaxOpenConns(c.config.MaxConns) + db.SetConnMaxIdleTime(c.config.ConnMaxIdleTime) + db.SetConnMaxIdleTime(c.config.ConnMaxLifetime) defaultVersion, _ := semver.Parse(defaultExpectedPostgreSQLVersion) version := &c.config.ExpectedVersion diff --git a/postgresql/provider.go b/postgresql/provider.go index 8bc7546d..9d9474fb 100644 --- a/postgresql/provider.go +++ b/postgresql/provider.go @@ -3,9 +3,11 @@ package postgresql import ( "context" "fmt" + "os" + "time" + "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/service/sts" - "os" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" @@ -21,8 +23,12 @@ import ( ) const ( - defaultProviderMaxOpenConnections = 20 - defaultExpectedPostgreSQLVersion = "9.0.0" + defaultProviderMaxOpenConnections = 20 // sql.DB Default + defaultProviderMaxIdleConnections = 0 + // Keep connection lifetimes short by default, to prevent blocking 'destroy' actions on database instance resources. + defaultProviderConnectionMaxIdleTime = time.Second * 5 + defaultProviderConnectionMaxLifetime = time.Second * 10 + defaultExpectedPostgreSQLVersion = "9.0.0" ) // Provider returns a terraform.ResourceProvider. @@ -192,6 +198,35 @@ func Provider() *schema.Provider { Description: "Maximum number of connections to establish to the database. Zero means unlimited.", ValidateFunc: validation.IntAtLeast(-1), }, + "max_idle_connections": { + Type: schema.TypeInt, + Optional: true, + Default: defaultProviderMaxIdleConnections, + Description: "Maximum number of idle connections to hold for the database. " + + "NOTE: Idle connections that aren't cleaned-up can cause problems if a database is destroyed in " + + "the same plan. The default is 0 to prevent that issue.", + ValidateFunc: validation.IntAtLeast(-1), + }, + "connection_max_idle_time": { + Type: schema.TypeString, + Optional: true, + Default: defaultProviderConnectionMaxIdleTime.String(), + Description: "Duration to hold idle connections to the database. Setting to 0 will hold connections " + "" + + "forever. " + + "NOTE: Idle connections that aren't cleaned-up can cause problems if a database is destroyed in " + + "the same plan. This should be > 0 to prevent that.", + ValidateFunc: validateParsableDuration, + }, + "connection_max_lifetime": { + Type: schema.TypeString, + Optional: true, + Default: defaultProviderConnectionMaxLifetime.String(), + Description: "Maximum lifetime of any connections to the database. Setting to 0 will hold " + + "connections forever. " + + "NOTE: Idle connections that aren't cleaned-up can cause problems if a database is destroyed in " + + "the same plan. This should be > 0 to prevent that.", + ValidateFunc: validateParsableDuration, + }, "expected_version": { Type: schema.TypeString, Optional: true, @@ -236,6 +271,13 @@ func validateExpectedVersion(v interface{}, key string) (warnings []string, erro return } +func validateParsableDuration(v any, _ string) (warnings []string, errors []error) { + if _, err := time.ParseDuration(v.(string)); err != nil { + errors = append(errors, fmt.Errorf("invalid duration (%q): %w", v.(string), err)) + } + return +} + func getRDSAuthToken(region string, profile string, role string, username string, host string, port int) (string, error) { endpoint := fmt.Sprintf("%s:%d", host, port) @@ -367,6 +409,10 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) { password = d.Get("password").(string) } + // Safe to ignore the error, since this is already checked in validation + connMaxIdleTime, _ := time.ParseDuration(d.Get("connection_max_idle_time").(string)) + connMaxLifetime, _ := time.ParseDuration(d.Get("connection_max_lifetime").(string)) + config := Config{ Scheme: d.Get("scheme").(string), Host: host, @@ -379,6 +425,9 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) { ApplicationName: "Terraform provider", ConnectTimeoutSec: d.Get("connect_timeout").(int), MaxConns: d.Get("max_connections").(int), + MaxIdleConns: d.Get("max_idle_connections").(int), + ConnMaxIdleTime: connMaxIdleTime, + ConnMaxLifetime: connMaxLifetime, ExpectedVersion: version, SSLRootCertPath: d.Get("sslrootcert").(string), GCPIAMImpersonateServiceAccount: d.Get("gcp_iam_impersonate_service_account").(string), From 3d72434535c516fe2bc2cd958d0d4980f671ae66 Mon Sep 17 00:00:00 2001 From: Aaron Tatar Date: Mon, 7 Apr 2025 12:31:17 +0200 Subject: [PATCH 2/2] Update documentation --- website/docs/index.html.markdown | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/website/docs/index.html.markdown b/website/docs/index.html.markdown index bc0a1b03..91d39e1d 100644 --- a/website/docs/index.html.markdown +++ b/website/docs/index.html.markdown @@ -172,6 +172,15 @@ The following arguments are supported: default is `180s`. Zero or not specified means wait indefinitely. * `max_connections` - (Optional) Set the maximum number of open connections to the database. The default is `20`. Zero means unlimited open connections. +* `max_idle_connections` - (Optional) Maximum number of idle connections to hold for the database. NOTE: Idle + connections that aren't cleaned-up can cause problems if a database is destroyed in the same plan. The default is 0 to + prevent that issue. +* `connection_max_idle_time` - (Optional) Duration to hold idle connections to the database. Setting to 0 will hold + connections forever. NOTE: Idle connections that aren't cleaned-up can cause problems if a database is destroyed in + the same plan. This should be > 0 to prevent that. +* `connection_max_lifetime` - (Optional) Maximum lifetime of any connections to the database. Setting to 0 will hold + connections forever. NOTE: Idle connections that aren't cleaned-up can cause problems if a database is destroyed in + the same plan. This should be > 0 to prevent that. * `expected_version` - (Optional) Specify a hint to Terraform regarding the expected version that the provider will be talking with. This is a required hint in order for Terraform to talk with an ancient version of PostgreSQL.