fix: Connection retry logic#576
Conversation
|
@cyrilgdn could you review this please? I'm right now forced to run a fork |
|
@cyrilgdn I checked the code, it looks good. Could you maybe give it a second look and merge it. Would be a highly appreciated featured. Thank you! |
There was a problem hiding this comment.
Pull request overview
This PR improves connection resilience in terraform-provider-postgresql by adding configurable retry behavior when establishing PostgreSQL connections, targeting intermittent failures (e.g., through proxies) as described in issue #495.
Changes:
- Adds provider configuration knobs for connection retries and retry timeout.
- Implements retry + ping validation during
Client.Connect()to reduce transient connection failures. - Introduces a configurable DB connection max lifetime and applies it to opened connections.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
postgresql/provider.go |
Adds new provider schema fields and wires them into the runtime Config. |
postgresql/config.go |
Adds retry-based connection establishment and applies connection max lifetime settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@atorrescogollo The feedback from copilot seems valid, could you work over the PR based on it? |
- Default conn_max_lifetime_seconds to 0 (unlimited) to preserve pre-retry-logic behavior for existing users. - Narrow dbRegistryLock scope so dialing/retrying no longer stalls Connect() calls for other DSNs; handle the publish race. - Close the leaked *sql.DB handle on each failed retry attempt. - Classify non-retryable errors (bad auth, unknown database) so retries fail fast instead of exhausting max_conn_retries. - Document zero-value behavior for max_conn_retries. - Add unit tests for connectWithRetry's attempt-counting behavior.
|
@simonfrey-pf can you review again? |
max_conn_retries now counts actual retries as documented ('zero means no
retries'): retryCount is incremented only when a retry is performed, so
MaxConnRetries=N yields 1 initial attempt plus N retries. Previously it
counted total attempts, one short of the documented behavior.
conn_max_lifetime_seconds validation tightened from IntAtLeast(-1) to
IntAtLeast(0), since SetConnMaxLifetime treats all non-positive values as
unlimited and -1 carried no distinct meaning.
|
@simonfrey-pf I think I addressed all your review comments. Can you review once again? |
simonfrey-pf
left a comment
simonfrey-pf
left a comment
There was a problem hiding this comment.
Apart from this one typo looks good to me
@cyrilgdn Could you check this PR and then LGTM :D Looking forward to have this feature, as in our stack it really is needed 😄
| connectRetryTimeout := time.Duration(c.config.ConnectionRetryTimeoutSeconds) * time.Second | ||
| retryError := retry.RetryContext(ctx, connectRetryTimeout, func() *retry.RetryError { | ||
| if c.config.Scheme == "postgres" { | ||
| db, err = sql.Open(proxyDriverName, dsn) | ||
| db, err = sql.Open(postgresDriverName, dsn) | ||
| } else if c.config.Scheme == "gcppostgres" && c.config.GCPIAMImpersonateServiceAccount != "" { | ||
| db, err = openImpersonatedGCPDBConnection(context.Background(), dsn, c.config.GCPIAMImpersonateServiceAccount) | ||
| db, err = openImpersonatedGCPDBConnection(ctx, dsn, c.config.GCPIAMImpersonateServiceAccount) | ||
| } else { | ||
| db, err = postgres.Open(context.Background(), dsn) | ||
| db, err = postgres.Open(ctx, dsn) | ||
| } | ||
|
|
||
| if err == nil { | ||
| err = db.Ping() | ||
| err = db.PingContext(ctx) | ||
| } |
There was a problem hiding this comment.
@cyrilgdn I think copilot is wrong here. Each attempt is already bounded by connect_timeout. RetryContext uses connection_retry_timeout_seconds as the total budget across retries
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@cyrilgdn Could we get this one over the finish linie? @atorrescogollo and me invested already quite some time to get the PR into shape. If there is anything else you would like us to change, please let us know 🙏 |
Fixes #495
This PR adds support for automatically retrying PostgreSQL connections in terraform-provider-postgresql, similar to the behavior already implemented in terraform-provider-mysql
Currently, unstable network issues can cause PostgreSQL connections to fail on the first attempt, leading to unnecessary errors. By adding retry logic, the provider becomes more resilient and reliable, especially in environments where the connection is unstable (for example, when accessing through a SOCKS proxy).