-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
postgres: expose database/sql tunables #1357
Conversation
1b67f8c
to
c94b383
Compare
@@ -88,6 +89,12 @@ type Postgres struct { | |||
SSL PostgresSSL `json:"ssl" yaml:"ssl"` | |||
|
|||
ConnectionTimeout int // Seconds | |||
|
|||
// database/sql tunables, see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would it be appropriate to set these to a higher value? (about how many clients?) What's the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaults right now should be zeros, which is the same as the stdlib default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realised I hadn't answered your question at all there, but it was time to go to bed. I'll update the issue #1354 with some details, as I'd think that's what people might find first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include defaults here that we feel are reasonable values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 sure. I'm not sure what makes for good defaults, but I can try 😅
storage/sql/sql.go
Outdated
@@ -55,6 +56,8 @@ func isRetryableSerializationFailure(err error) bool { | |||
return false | |||
} | |||
|
|||
const maxRetries = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we already added retries in a previous PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(rebased away; but #1356 hasn't gone in yet..)
c94b383
to
b600e3e
Compare
The defaults are getting messed up by this right now. Will push another commit. |
b600e3e
to
223ab85
Compare
✔️ Now, if these are not provided in the config, they'll not be set. So, I've looked into adding further tests around the connection counts, and came up with a tests here: diff --git a/storage/sql/postgres_test.go b/storage/sql/postgres_test.go
index a207739..64d6fef 100644
--- a/storage/sql/postgres_test.go
+++ b/storage/sql/postgres_test.go
@@ -3,8 +3,12 @@
package sql
import (
+ "context"
+ "database/sql"
"os"
+ "sync"
"testing"
+ "time"
)
func TestPostgresTunables(t *testing.T) {
@@ -46,4 +50,44 @@ func TestPostgresTunables(t *testing.T) {
t.Errorf("expected MaxOpenConnections to be set to 101, got %d", m)
}
})
+
+ t.Run("attempt to open max+1 connections stalls", func(t *testing.T) {
+ cfg := *baseCfg
+ max := 2
+ cfg.MaxOpenConns = &max
+ c, err := cfg.open(logger, cfg.createDataSourceName())
+ if err != nil {
+ t.Fatalf("error opening connector: %s", err.Error())
+ }
+ defer c.db.Close()
+ done := sync.WaitGroup{}
+ done.Add(1)
+ hogs := sync.WaitGroup{}
+ hogs.Add(max)
+ hogConnection := func(db *sql.DB) {
+ r, err := db.Query("SELECT 1")
+ if err != nil {
+ return
+ }
+ hogs.Done()
+ done.Wait()
+ r.Close() // not closing the rows makes it occupy the connection
+ }
+ for i := 0; i < max; i++ {
+ go hogConnection(c.db)
+ }
+ hogs.Wait()
+ if n := c.db.Stats().OpenConnections; n != max {
+ t.Fatalf("expected %d open connections, found %d", max, n)
+ }
+
+ ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+ defer cancel()
+ _, err = c.db.QueryContext(ctx, "SELECT 1")
+ if err != context.DeadlineExceeded {
+ t.Errorf("expected context deadline exceeded for (max+1)th connection, got %v", err)
+ }
+
+ done.Done() // release it all
+ })
} but since this would be testing |
storage/sql/config.go
Outdated
|
||
// database/sql tunables, see | ||
// https://golang.org/pkg/database/sql/#DB.SetConnMaxLifetime and below | ||
// using pointers to differentiate "unset" (nil) from 0 (int's zero value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Differentiating between zero and nil is hairy. Can we use 0 to set a reasonable default?
- adapted TestUnmarshalConfig to ensure the fields are read in - added a test to see that at least MaxOpenConns works: - this is only exposed through (*db).Stats() in go 1.11, so this test has a build tag - the other two configurables can't be read back, so we've got to trust that the mechanism works given the one instance that's tested.. Signed-off-by: Stephan Renatus <srenatus@chef.io>
223ab85
to
73fdf4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
postgres: expose database/sql tunables Fixes dexidp#1354. I've not had a need for MaxIdleConns and ConnMaxLifetime myself, but it felt weird to only expose of the three settings.
This is for #1354.
I've not had a need for
MaxIdleConns
andConnMaxLifetime
myself, but it felt weird to only expose of the three settings.