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

Register dialer #470

Closed
tianzhou opened this issue Jun 21, 2016 · 7 comments
Closed

Register dialer #470

tianzhou opened this issue Jun 21, 2016 · 7 comments

Comments

@tianzhou
Copy link

It would be helpful if pq has something similar as mysql.RegisterDial. Thus the custom protocol can be still accessed via the standard sql interface.

One idea is to add a new attribute "protocol=XXX" to the DSN to allow client to specify the dialer.

I could try to make a PR if this sounds good to you.

@johto
Copy link
Contributor

johto commented Jun 21, 2016

What's wrong with the custom dialing we already have?

@tianzhou
Copy link
Author

We can't use the standard sql.open for custom dialing, which means the code can't leverage all the features the sql driver infrastructure provides (e.g. connection pooling, various tests)

@johto
Copy link
Contributor

johto commented Jun 21, 2016

We can't use the standard sql.open for custom dialing

Why not?

I'm not sure if it's documented anywhere, but the way to use a custom dialer is to wrap pq and register your own driver that uses pq's DialOpen function.

@tianzhou
Copy link
Author

Doing a wrapper would work, but that's kind of duplicate work since it requires doing the dsn parsing which is already implemented in pg. Ideally, client should only need to implement a custom dialer.

I personally feel providing dialer registration will make pq more extensible and work better with sql.driver.

Maybe I miss some other implications of introducing dialer registration to pg?

@johto
Copy link
Contributor

johto commented Jun 21, 2016

Doing a wrapper would work, but that's kind of duplicate work since it requires doing the dsn parsing which is already implemented in pg.

Why do you think it requires that?

But in any case, I think that problem can be solved by just exporting the logic; see #375.

Maybe I miss some other implications of introducing dialer registration to pg?

I think you're still confused about the interface that already exists. What do you want to do that you couldn't do by building on top of this wrapper?

package main

import (
    "database/sql"
    "database/sql/driver"
    "github.com/lib/pq"
    "log"
    "net"
    "time"
)

type drvWrapper struct{}

func (d drvWrapper) Open(name string) (driver.Conn, error) {
    return pq.DialOpen(drvWrapper{}, name)
}

func (d drvWrapper) Dial(network, address string) (net.Conn, error) {
    return net.Dial(network, address)
}

func (d drvWrapper) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) {
    return net.DialTimeout(network, address, timeout)
}

func init() {
    sql.Register("myPgDialer", drvWrapper{})
}

func main() {
    db, err := sql.Open("myPgDialer", "sslmode=disable")
    if err != nil {
        log.Fatal(err)
    }
    err = db.Ping()
    if err != nil {
        log.Fatal(err)
    }
}

@tianzhou
Copy link
Author

I could do it in a wrapper, then we have to write a new driver for every custom dialer. This means we couple the dialer with driver, which is unnecessary. Introducing dialer registration will make pq driver pluggable with different dialers.

#375 also requires to implement a driver.

@ernestoalejo
Copy link

I'm using https://upper.io/db.v3. I need to use a custom dialer and the name of the driver is hard-coded in the library and can't be registered twice: https://github.com/upper/db/blob/master/postgresql/database.go#L122

Admittedly it can be also be implemented in upper, but it won't be the first nor the last wrapper around this library that does it. Registering a custom dialer would simplify the future setup for App Engine for example, allowing the final user to do this: https://github.com/go-sql-driver/mysql/blob/master/appengine.go (I presume they will add support for Postgre when it comes out of beta)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants