Skip to content
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

Merged
merged 1 commit into from
Dec 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions cmd/dex/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@ func TestUnmarshalConfig(t *testing.T) {
rawConfig := []byte(`
issuer: http://127.0.0.1:5556/dex
storage:
type: sqlite3
type: postgres
config:
file: examples/dex.db

host: 10.0.0.1
port: 65432
maxOpenConns: 5
maxIdleConns: 3
connMaxLifetime: 30
connectionTimeout: 3
web:
http: 127.0.0.1:5556
staticClients:
Expand Down Expand Up @@ -69,9 +73,14 @@ logger:
want := Config{
Issuer: "http://127.0.0.1:5556/dex",
Storage: Storage{
Type: "sqlite3",
Config: &sql.SQLite3{
File: "examples/dex.db",
Type: "postgres",
Config: &sql.Postgres{
Host: "10.0.0.1",
Port: 65432,
MaxOpenConns: 5,
MaxIdleConns: 3,
ConnMaxLifetime: 30,
ConnectionTimeout: 3,
},
},
Web: Web{
Expand Down
25 changes: 25 additions & 0 deletions storage/sql/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"regexp"
"strconv"
"strings"
"time"

"github.com/lib/pq"
sqlite3 "github.com/mattn/go-sqlite3"
Expand Down Expand Up @@ -88,6 +89,13 @@ type Postgres struct {
SSL PostgresSSL `json:"ssl" yaml:"ssl"`

ConnectionTimeout int // Seconds

// database/sql tunables, see
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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 😅

// https://golang.org/pkg/database/sql/#DB.SetConnMaxLifetime and below
// Note: defaults will be set if these are 0
MaxOpenConns int // default: 5
MaxIdleConns int // default: 5
ConnMaxLifetime int // Seconds, default: not set
}

// Open creates a new storage implementation backed by Postgres.
Expand Down Expand Up @@ -177,6 +185,23 @@ func (p *Postgres) open(logger logrus.FieldLogger, dataSourceName string) (*conn
return nil, err
}

// set database/sql tunables if configured
if p.ConnMaxLifetime != 0 {
db.SetConnMaxLifetime(time.Duration(p.ConnMaxLifetime) * time.Second)
}

if p.MaxIdleConns == 0 {
db.SetMaxIdleConns(5)
} else {
db.SetMaxIdleConns(p.MaxIdleConns)
}

if p.MaxOpenConns == 0 {
db.SetMaxOpenConns(5)
} else {
db.SetMaxOpenConns(p.MaxOpenConns)
}

errCheck := func(err error) bool {
sqlErr, ok := err.(*pq.Error)
if !ok {
Expand Down
48 changes: 48 additions & 0 deletions storage/sql/postgres_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// +build go1.11

package sql

import (
"os"
"testing"
)

func TestPostgresTunables(t *testing.T) {
host := os.Getenv(testPostgresEnv)
if host == "" {
t.Skipf("test environment variable %q not set, skipping", testPostgresEnv)
}
baseCfg := &Postgres{
Database: getenv("DEX_POSTGRES_DATABASE", "postgres"),
User: getenv("DEX_POSTGRES_USER", "postgres"),
Password: getenv("DEX_POSTGRES_PASSWORD", "postgres"),
Host: host,
SSL: PostgresSSL{
Mode: sslDisable, // Postgres container doesn't support SSL.
}}

t.Run("with nothing set, uses defaults", func(t *testing.T) {
cfg := *baseCfg
c, err := cfg.open(logger, cfg.createDataSourceName())
if err != nil {
t.Fatalf("error opening connector: %s", err.Error())
}
defer c.db.Close()
if m := c.db.Stats().MaxOpenConnections; m != 5 {
t.Errorf("expected MaxOpenConnections to have its default (5), got %d", m)
}
})

t.Run("with something set, uses that", func(t *testing.T) {
cfg := *baseCfg
cfg.MaxOpenConns = 101
c, err := cfg.open(logger, cfg.createDataSourceName())
if err != nil {
t.Fatalf("error opening connector: %s", err.Error())
}
defer c.db.Close()
if m := c.db.Stats().MaxOpenConnections; m != 101 {
t.Errorf("expected MaxOpenConnections to be set to 101, got %d", m)
}
})
}