I'm not sure I understand. @kardianos?

Can't drivers define their own opener funcs in their own packages already?

Let me think about this and look into your motivating examples.

@james-lawrence If I understand you correctly, you are looking for a way to create a connection pool (*sql.DB) but still be able to pass in option parameters when you are creating it. The following isn't a serious proposal, but a rephasing to ensure I understand what you feel is missing:

package sql

func OpenOptions(name string, opts driver.OpenerOptions) (*DB, error) {...}

package driver

// OpenerOptions is satisfied by a custom driver configuration structure.
type OpenerOptions interface{
    DataSourceName() string
}

// DriverOptions may be implemented by the Driver to allow configuring with specific value in addition to
// a DSN.
type DriverOptions interface {
    Driver

    OpenOptions(OpenerOptions) (Conn, error)
}

package mysql

type DBOptions struct {
    DSN string
    TLSConfig *tls.Config
}
func (opts *DBOptions) DataSourceName() string { return opts.DSN }

Is this another way of expressing your requested end state? Again, this is stated mainly for my own understanding, not a proposal at this time.

it could be viewed that way yes.

Thanks for confirming.

Before considering this further, I would want some driver maintainers to weigh in on this. Can you link to discussions with driver maintainers about this? Or ask them to comment on this issue?

sure thing. I'll point them in this direction.

Waiting for more input.

sorry, didn't get around to getting to other driver maintainers this weekend.

Without weighing in on any of the proposed solutions, I can confirm that this is an issue for pgx. At the moment there are a number of options that can't be easily be handled with a database URL or DSN such as TLS configuration, logging, custom dialer, and an after connect hook.

In the current version of pgx, the approach of registering a new driver for each different config was used. However, there is no way to unregister a driver. This means the driver and the associated pgx data structures can never be garbage collected. For one user who was apparently opening and closing databases a great many times this caused a problem (unusual behavior to be sure, but it still shouldn't break).

In the upcoming version, I'm taking an approach similar to the MySQL driver in that pgx maintains a configuration registry and can encode a reference to that registry in a connection string. Then when Driver.Open is called the configuration can be extracted.

The usage looks like so:

driverConfig := stdlib.DriverConfig{
	ConnConfig: pgx.ConnConfig{
		Logger:   logger,
	},
}

stdlib.RegisterDriverConfig(&driverConfig)

db, err := sql.Open("pgx", driverConfig.ConnectionString("postgres://pgx_md5:secret@127.0.0.1:5432/pgx_test"))

@jackc If SQLite, pgx, and mysql all use workarounds for the lack of parameters to pass in, I'm interested in thoughts any of the database drivers have in a less hacky way to pass in params.

Basically, I'm +1 on this proposal.

Another API design option is prohibit DSN. Driver is fully configured at beginning.
No DSN parsing on every connection.

type Connector func() (driver.Conn, error)

func Open(driverName, dataSourceName string) (*DB, error) {
	driversMu.RLock()
	driveri, ok := drivers[driverName]
	driversMu.RUnlock()
	if !ok {
		return nil, fmt.Errorf("sql: unknown driver %q (forgotten import?)", driverName)
	}
	return NewFromConnector(func() (driver.Conn, error) { return driveri.Open(dataSourceName) }), nil
}

func NewFromConnector(c Connector) *DB {
	db := &DB{
		connector:     c,
		openerCh:     make(chan struct{}, connectionRequestQueueSize),
		lastPut:      make(map[*driverConn]string),
		connRequests: make(map[uint64]chan connRequest),
	}
	go db.connectionOpener()
	return db
}

I think @methane's API would do everything pgx would need.

@kardianos anything else we need to do here to move forward? I'm happy to do the work for this.

Feel free to send a CL, though it won't be able to be merged until the tree opens again.

It sounds like @kardianos is OK with @methane's proposal of adding type Connector and func NewFromConnector, although maybe that should have a shorter name. I am a little confused about this comment though:

Another API design option is prohibit DSN. Driver is fully configured at beginning. No DSN parsing on every connection.

To be clear, we cannot break all the code that is using Open with syntaxes that exist today. Assuming we're not talking about breaking that code, and based on @kardianos being happy, proposal accepted.

Change https://golang.org/cl/53430 mentions this issue: database/sql: add OpenDB to directly create a *DB without a DSN.

I also strongly support @methane's general approach.

While I don't see any benefit in the proposal in the first post (one still has to use different driver instances, which seems like a bad workaround to me, if I understand it correctly?), the other proposal is very flexible and also avoids useless DSN string parsing (and thus also allocations) every time a connection is opened.

In the MySQL driver we already provide a Config struct, which we could easily extend by a function which implements the proposed simple Connector interface. Then a user could simply adjust the struct fields and then pass it to the database/sql package to open a new connection.

If this is the intended approach, I would however define the Connector interface a little different:

type Connector interface {
        Connect() (driver.Conn, error)
}

This way a user could directly pass the instance of the Config struct instead of a method defined on it. I think this interface definition makes more sense as it has little benefit to call the Connect() function without any data containing the connection options attached to it.

Edit: I just saw that https://golang.org/cl/53430 is already pretty much that. +1 for that CL in that case.

We should also provide way to stop connectionOpener.

@mattn Do you mean adding Context like this?

type Connector interface {
        Connect(ctx context.Context) (driver.Conn, error)
}

@methane Yes, it is.

Not sure I agree with that, the drivers could just as easily allow a ConnectTimeout configuration option and handle it internally. no reason to expose a context in this manner.

I'm +0.5 to adding context to Connect().

Current database/sql won't use context for opening connection, because connections
are created asynchronously in the connectionOpener goroutine.

But if future database/sql adds support bypassing pool and use connection synchronously,
it can be beneficial.
It will be good for GAE/Go because GAE/Go requires request context to open and use TCP connection.

But I don't know Google's future plan.

Could you sketch/expand on that concept of a sync open that bypasses the pool?

For user level, it's very similar to db.Conn(ctx)

conn, err := db.NewConn(ctx)  // same to db.Conn(ctx), but always return new connection.
...
conn.Close() // unlike db.Conn(ctx), conn.Close() closes the connection

db.NewConn(ctx) uses db.connector.Connect(ctx), without touching pool.

What is the advantage over having the individual driver just implement this? why does it belong in the sql package?

edit: I'm just curious why this is useful? and why it should be implemented by the sql.DB when Conn(ctx) already exists? I don't see any added benefit.

As I noted above, GAE/Go has special requirements: context bound to request is needed to use TCP.
Current db.Conn(ctx) uses ctx only for timeout. Connection got doesn't bound to the context.
So, database/sql cannot be used for TCP based protocol on GAE/Go for now.

Another reason is flexibility. Currently, database/sql provides pool and driver abstraction.
But pool behavior may match user's requirement not completely.
With this API, user can implement own pool to match there requirement.

But I'm not sure it's enough reason or not. So I'm only +0.5.

@methane I don't think any sort of database pool will work on GAE classic. As you noted, the socket type requires a valid Go context, but contexts are scoped to a request. So in this case you wouldn't open a connection pool, just individual connections as you needed them. However, GAE flexible removes this restriction and you can use the standard "database/sql" package as it is today.

I'm not currently too hot to add db.NewConn(ctx), though it is interesting to be honest (allowing the user to create a connection outside the connection pool). I would need a more convincing use case before it gets added however.

I am interested in adding context to Connect. I'm not sure if it will be useful or not, so I'll need to look at the code some more to determine that.

FYI, I'm happy with the CL. I may merge it and decide about if Connect should have a context in the next month.

I'm recommending that Connect include a context. There are two current paths that open a connection: a background opener without a context (no associated request) and a open/connect command inline with the request with a context (because it does come from a request). This will allow the driver to abort a dial if the context is cancled and return promptly.

What's the benefit of cancelling the connection instead of just putting it in the pool if it isn't needed anymore when it is finally open?
The same might happen with the next request, but then a connection from the pool is available.

Unless I miss something here, adding a Context would only make sense if there is some way to open a single connection directly.

I assume that by "client" you mean the user of the database/sql interface (and not the driver itself)?

You are right, a context makes sense in situations where no connection is available and a new connection is opened synchronously.
Ideally, the function would return without waiting for the driver, while the driver asynchronously keeps establishing the connection which then is put in the pool when it is open.
Otherwise it might lead to situations where the driver is repeatedly asked to open new connections which are then cancelled, resulting in a lot of wasted resources. But I doubt that that could be implemented in a useful way, as the connection then had to be always opened by another goroutine, adding unnecessary synchronization overhead.

so verdict is to pass on adding the context? I agree with julien's statements it being a pain to implement in a useful manner.

The verdict is to add Context as in Daniel's comments on the CL.
The driver-side implementation would be to simply use net.DialContext and pass the Context to it.