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

proposal: database/sql: support creating a *sql.DB directly from a driver.Driver and dsn. #20268

Closed
james-lawrence opened this issue May 6, 2017 · 35 comments

Comments

@james-lawrence
Copy link
Contributor

@james-lawrence james-lawrence commented May 6, 2017

sometimes drivers expose functionality that doesn't make sense to handle via the DSN and would be painful/impossible to setup, using PGX as an example:

splitting sql.Open into two methods solves the above issues with minimal changes to the runtime.

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 NewFromDriver(driveri, dataSourceName), nil
}

func NewFromDriver(driveri driver.Driver, dataSourceName string) *DB {
	db := &DB{
		driver:       driveri,
		dsn:          dataSourceName,
		openerCh:     make(chan struct{}, connectionRequestQueueSize),
		lastPut:      make(map[*driverConn]string),
		connRequests: make(map[uint64]chan connRequest),
	}
	go db.connectionOpener()
	return db
}

for reference here is the current open code:

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)
	}

	db := &DB{
		driver:       driveri,
		dsn:          dataSourceName,
		openerCh:     make(chan struct{}, connectionRequestQueueSize),
		lastPut:      make(map[*driverConn]string),
		connRequests: make(map[uint64]chan connRequest),
	}

	go db.connectionOpener()
	return db, nil
}
@bradfitz bradfitz changed the title proposal database/sql - support creating a *sql.DB directly from a driver.Driver and dsn. proposal: database/sql: support creating a *sql.DB directly from a driver.Driver and dsn. May 7, 2017
@gopherbot gopherbot added this to the Proposal milestone May 7, 2017
@gopherbot gopherbot added the Proposal label May 7, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 7, 2017

I'm not sure I understand. @kardianos?

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

@kardianos
Copy link
Contributor

@kardianos kardianos commented May 8, 2017

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

@kardianos kardianos self-assigned this May 8, 2017
@kardianos
Copy link
Contributor

@kardianos kardianos commented May 8, 2017

@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.

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented May 8, 2017

it could be viewed that way yes.

@kardianos
Copy link
Contributor

@kardianos kardianos commented May 8, 2017

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?

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented May 8, 2017

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

@rsc
Copy link
Contributor

@rsc rsc commented May 15, 2017

Waiting for more input.

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented May 15, 2017

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

@jackc
Copy link

@jackc jackc commented May 16, 2017

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"))
@kardianos
Copy link
Contributor

@kardianos kardianos commented May 16, 2017

@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.

@methane
Copy link
Contributor

@methane methane commented May 17, 2017

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
}
@jackc
Copy link

@jackc jackc commented May 17, 2017

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

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Jun 29, 2017

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

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jun 29, 2017

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

@rsc
Copy link
Contributor

@rsc rsc commented Jul 17, 2017

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.

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Jul 17, 2017

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 5, 2017

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

@julienschmidt
Copy link
Contributor

@julienschmidt julienschmidt commented Aug 17, 2017

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.

@mattn
Copy link
Member

@mattn mattn commented Aug 21, 2017

We should also provide way to stop connectionOpener.

@methane
Copy link
Contributor

@methane methane commented Aug 21, 2017

@mattn Do you mean adding Context like this?

type Connector interface {
        Connect(ctx context.Context) (driver.Conn, error)
}
@mattn
Copy link
Member

@mattn mattn commented Aug 21, 2017

@methane Yes, it is.

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Aug 24, 2017

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.

@methane
Copy link
Contributor

@methane methane commented Aug 24, 2017

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.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 26, 2017

@methane
Copy link
Contributor

@methane methane commented Aug 27, 2017

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.

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Aug 27, 2017

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.

@methane
Copy link
Contributor

@methane methane commented Aug 28, 2017

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.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 31, 2017

@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.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 31, 2017

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.

@julienschmidt
Copy link
Contributor

@julienschmidt julienschmidt commented Aug 31, 2017

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.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 31, 2017

@julienschmidt
Copy link
Contributor

@julienschmidt julienschmidt commented Aug 31, 2017

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.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 31, 2017

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Sep 1, 2017

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.

@julienschmidt
Copy link
Contributor

@julienschmidt julienschmidt commented Sep 1, 2017

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.

@gopherbot gopherbot closed this in e6358c7 Sep 23, 2017
@at15 at15 mentioned this issue Jan 12, 2018
2 of 8 tasks complete
@golang golang locked and limited conversation to collaborators Sep 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.