code review 5706047: database/sql/driver: add a argument to sql.Open and Dri...

database/sql/driver: add a argument to sql.Open and Driver.Open.

There is a requirement that executing something after database connected.
Eg. 'SET NAMES utf8' for mysql. Currently, there is no use to execute this.
Because "database/sql uses multiple simultaneous connections to database"(mymysql, Ziutek)

Option 1:
Db.PostConn set the things, that will be executed after Db.driver.Open
was called.

db, err := sql.Open("mymysql", "test/testuser/TestPasswd9")
db.PostConn("set names utf8") // without this statment, the Chinese characters will be displayed as '0x3F'
rows, err := db.Query("SELECT * FROM go")

Option 2:
Let Driver.Open accept arguments: []Value.
We colud pass the arguments to the drivers,
then let the drivers determine what should be done.

[mikespook, 2012-02-28 16:38:26]

database/sql: added the Db.PostConn for executive something after connected.

There is a requirement that executing something after database connected.
Eg. 'SET NAMES utf8' for mysql. Currently, there is no use to execute this.
Because "database/sql uses multiple simultaneous connections to
database"(mymysql, Ziutek)
Db.PostConn set the things, that will be executed after Db.driver.Open was
called.

db, err := sql.Open("mymysql", "test/testuser/TestPasswd9")
db.PostConn("set names utf8") // without this statment, the Chinese characters
will be displayed as '0x3F'
rows, err := db.Query("SELECT * FROM go")

[mikespook, 2012-02-28 16:41:04]

Hello golang-dev@googlegroups.com (cc: golang-china@googlegroups.com,
golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[mikespook, 2012-02-28 16:44:09]

Hello golang-dev@googlegroups.com (cc: golang-china@googlegroups.com,
golang-dev@googlegroups.com),

Please take another look.

[mikespook, 2012-02-28 16:56:32]

Why the review didn't display in the group of golang-dev? Let me try again.

database/sql: added the Db.PostConn for executive something after connected.

There is a requirement that executing something after database connected.
Eg. 'SET NAMES utf8' for mysql. Currently, there is no use to execute this.
Because "database/sql uses multiple simultaneous connections to
database"(mymysql, Ziutek)
Db.PostConn set the things, that will be executed after Db.driver.Open was
called.

db, err := sql.Open("mymysql", "test/testuser/TestPasswd9")
db.PostConn("set names utf8") // without this statment, the Chinese characters
will be displayed as '0x3F'
rows, err := db.Query("SELECT * FROM go")

[vcc, 2012-02-28 17:01:57]

在 2012年2月29日星期三， 写道：

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> database/sql: added the Db.PostConn for executive something after
> connected.
>
> There is a requirement that executing something after database
> connected.
> Eg. 'SET NAMES utf8' for mysql. Currently, there is no use to execute
> this.


You can just add something to open name string, for example:
sql.Open("....","db/user/password?encoding=utf-8")
only need modify mymysql, don't need change database/sql, should be better.


Because "database/sql uses multiple simultaneous connections to
> database"(mymysql, Ziutek)
> Db.PostConn set the things, that will be executed after Db.driver.Open
> was called.
>
> db, err := sql.Open("mymysql", "test/testuser/TestPasswd9")
> db.PostConn("set names utf8") // without this statment, the Chinese
> characters will be displayed as '0x3F'
> rows, err := db.Query("SELECT * FROM go")
>
> Description:
> database/sql: added the Db.PostConn for executive something after
> connected.
>
> There is a requirement that executing something after database
> connected.
> Eg. 'SET NAMES utf8' for mysql. Currently, there is no use to execute
> this.
> Because "database/sql uses multiple simultaneous connections to
> database"(mymysql, Ziutek)
> Db.PostConn set the things, that will be executed after Db.driver.Open
> was called.
>
> db, err := sql.Open("mymysql", "test/testuser/TestPasswd9")
> db.PostConn("set names utf8") // without this statment, the Chinese
> characters will be displayed as '0x3F'
> rows, err := db.Query("SELECT * FROM go")
>
> Please review this at
http://codereview.appspot.com/**5706047/<http://codereview.appspot.com/5706047/>
>
> Affected files:
>  M src/pkg/database/sql/sql.go
>
>
> Index: src/pkg/database/sql/sql.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql.go
> +++ b/src/pkg/database/sql/sql.go
> @@ -179,9 +179,11 @@
>        driver driver.Driver
>        dsn    string
>
> -       mu       sync.Mutex // protects freeConn and closed
> -       freeConn []driver.Conn
> -       closed   bool
> +       mu            sync.Mutex // protects freeConn and closed
> +       freeConn      []driver.Conn
> +       closed        bool
> +       postConnQuery string
> +       postConnArgs  []interface{}
>  }
>
>  // Open opens a database specified by its database driver name and a
> @@ -198,6 +200,12 @@
>        return &DB{driver: driver, dsn: dataSourceName}, nil
>  }
>
> +// query will be executed after a conn was connected.
> +func (db *DB) PostConn(query string, args ...interface{}) {
> +       db.postConnQuery = query
> +       db.postConnArgs = args
> +}
> +
>  // Close closes the database, releasing any open resources.
>  func (db *DB) Close() error {
>        db.mu.Lock()
> @@ -235,7 +243,29 @@
>                return conn, nil
>        }
>        db.mu.Unlock()
> -       return db.driver.Open(db.dsn)
> +       ci, err := db.driver.Open(db.dsn)
> +       if err != nil {
> +               return nil, err
> +       }
> +
> +       if db.postConnQuery == "" && db.postConnArgs == nil {
> +               return ci, nil
> +       }
> +       if c, ok := ci.(driver.Conn); ok {
> +               stmt, err := c.Prepare(db.postConnQuery)
> +               if err != nil {
> +                       if err != driver.ErrSkip {
> +                               if err != nil {
> +                                       return nil, err
> +                               }
> +                       }
> +               }
> +               _, err = stmt.Exec(db.postConnArgs)
> +               if err != nil {
> +                       return nil, err
> +               }
> +       }
> +       return ci, nil
>  }
>
>  func (db *DB) connIfFree(wanted driver.Conn) (conn driver.Conn, ok bool) {
>
>
>

[mikespook, 2012-02-28 17:18:11]

It means that we must change the interface in the
database/sql/driver/driver.go:36, from:

Driver.Open(name string) (Conn, error)

to:

Driver.Open(name string, args...Value) (Conn, error)

Should we modify the interface or just add a method to the entrance?
To be truth, I prefer modify the interface.


于 Wed, 29 Feb 2012 01:01:57 +0800
Wei guangjing <vcc.163@gmail.com> 写道:

> 在 2012年2月29日星期三， 写道：
> 
> > Reviewers: golang-dev_googlegroups.com,
> >
> > Message:
> > database/sql: added the Db.PostConn for executive something after
> > connected.
> > 
> 
> You can just add something to open name string, for example:
> sql.Open("....","db/user/password?encoding=utf-8")
> only need modify mymysql, don't need change database/sql, should be
> better.
> 

[rsc, 2012-02-28 18:04:05]

R+=bradfitz
This is an API change.

[bradfitz, 2012-02-28 22:39:04]

I think this is something drivers should provide.  You can pack any form of
arguments into the driver.Open(name string).  That name string can be any
format.

In short, I don't think this requires any database/sql* changes.

On Tue, Feb 28, 2012 at 8:38 AM, <mikespook@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> database/sql: added the Db.PostConn for executive something after
> connected.
>
> There is a requirement that executing something after database
> connected.
> Eg. 'SET NAMES utf8' for mysql. Currently, there is no use to execute
> this.
> Because "database/sql uses multiple simultaneous connections to
> database"(mymysql, Ziutek)
> Db.PostConn set the things, that will be executed after Db.driver.Open
> was called.
>
> db, err := sql.Open("mymysql", "test/testuser/TestPasswd9")
> db.PostConn("set names utf8") // without this statment, the Chinese
> characters will be displayed as '0x3F'
> rows, err := db.Query("SELECT * FROM go")
>
> Description:
> database/sql: added the Db.PostConn for executive something after
> connected.
>
> There is a requirement that executing something after database
> connected.
> Eg. 'SET NAMES utf8' for mysql. Currently, there is no use to execute
> this.
> Because "database/sql uses multiple simultaneous connections to
> database"(mymysql, Ziutek)
> Db.PostConn set the things, that will be executed after Db.driver.Open
> was called.
>
> db, err := sql.Open("mymysql", "test/testuser/TestPasswd9")
> db.PostConn("set names utf8") // without this statment, the Chinese
> characters will be displayed as '0x3F'
> rows, err := db.Query("SELECT * FROM go")
>
> Please review this at
http://codereview.appspot.com/**5706047/<http://codereview.appspot.com/5706047/>
>
> Affected files:
>  M src/pkg/database/sql/sql.go
>
>
> Index: src/pkg/database/sql/sql.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql.go
> +++ b/src/pkg/database/sql/sql.go
> @@ -179,9 +179,11 @@
>        driver driver.Driver
>        dsn    string
>
> -       mu       sync.Mutex // protects freeConn and closed
> -       freeConn []driver.Conn
> -       closed   bool
> +       mu            sync.Mutex // protects freeConn and closed
> +       freeConn      []driver.Conn
> +       closed        bool
> +       postConnQuery string
> +       postConnArgs  []interface{}
>  }
>
>  // Open opens a database specified by its database driver name and a
> @@ -198,6 +200,12 @@
>        return &DB{driver: driver, dsn: dataSourceName}, nil
>  }
>
> +// query will be executed after a conn was connected.
> +func (db *DB) PostConn(query string, args ...interface{}) {
> +       db.postConnQuery = query
> +       db.postConnArgs = args
> +}
> +
>  // Close closes the database, releasing any open resources.
>  func (db *DB) Close() error {
>        db.mu.Lock()
> @@ -235,7 +243,29 @@
>                return conn, nil
>        }
>        db.mu.Unlock()
> -       return db.driver.Open(db.dsn)
> +       ci, err := db.driver.Open(db.dsn)
> +       if err != nil {
> +               return nil, err
> +       }
> +
> +       if db.postConnQuery == "" && db.postConnArgs == nil {
> +               return ci, nil
> +       }
> +       if c, ok := ci.(driver.Conn); ok {
> +               stmt, err := c.Prepare(db.postConnQuery)
> +               if err != nil {
> +                       if err != driver.ErrSkip {
> +                               if err != nil {
> +                                       return nil, err
> +                               }
> +                       }
> +               }
> +               _, err = stmt.Exec(db.postConnArgs)
> +               if err != nil {
> +                       return nil, err
> +               }
> +       }
> +       return ci, nil
>  }
>
>  func (db *DB) connIfFree(wanted driver.Conn) (conn driver.Conn, ok bool) {
>
>
>

[mikespook, 2012-02-29 01:45:51]

Hello bradfitz@golang.org, vcc.163@gmail.com, rsc@golang.org (cc:
golang-china@googlegroups.com, golang-dev@googlegroups.com),

Please take another look.

[mikespook, 2012-02-29 01:50:08]

Hello bradfitz@golang.org, vcc.163@gmail.com, rsc@golang.org (cc:
golang-china@googlegroups.com, golang-dev@googlegroups.com),

Please take another look.

[mikespook, 2012-02-29 01:51:50]

于 Tue, 28 Feb 2012 14:39:04 -0800
Brad Fitzpatrick <bradfitz@golang.org> 写道:

> I think this is something drivers should provide.  You can pack any
> form of arguments into the driver.Open(name string).  That name
> string can be any format.
> 
> In short, I don't think this requires any database/sql* changes.
> 

So, means do this:

db, err := sql.Open("mymysql", "hms/root/xxiyy/set names utf8")

and then mymysql will explain the dns in it own codes.

Bad smell, isn't it?

In the Chinese or any other multi-byte character languages, do something
(eg. set the char-set or encoder) after connected is most likely a
standard action.

IMHO. And if the interface didn't tell drivers how to do it. There will
be lots of tricks were done by drivers.

I also think about this:

db, err := sql.Open("mymysql", "hms/root/xxiyy", []driver.Value{"set
names utf8"})

[MC.Spring, 2012-02-29 02:50:16]

I would prefer to use Query String model, suck as: 

db, err := sql.Open('mymysql', 'user=Username&passwd=Password&charset=UTF8')

And everything should be done in driver but not interface?

On 2012/02/29 01:51:50, mikespook wrote:
> 于 Tue, 28 Feb 2012 14:39:04 -0800
> Brad Fitzpatrick <mailto:bradfitz@golang.org> 写道:
> 
> > I think this is something drivers should provide.  You can pack any
> > form of arguments into the driver.Open(name string).  That name
> > string can be any format.
> > 
> > In short, I don't think this requires any database/sql* changes.
> > 
> 
> So, means do this:
> 
> db, err := sql.Open("mymysql", "hms/root/xxiyy/set names utf8")
> 
> and then mymysql will explain the dns in it own codes.
> 
> Bad smell, isn't it?
> 
> In the Chinese or any other multi-byte character languages, do something
> (eg. set the char-set or encoder) after connected is most likely a
> standard action.
> 
> IMHO. And if the interface didn't tell drivers how to do it. There will
> be lots of tricks were done by drivers.
> 
> I also think about this:
> 
> db, err := sql.Open("mymysql", "hms/root/xxiyy", []driver.Value{"set
> names utf8"})

[mikespook, 2012-02-29 03:18:05]

On 2012/02/29 02:50:16, MC.Spring wrote:
> I would prefer to use Query String model, suck as: 
> 
> db, err := sql.Open('mymysql', 'user=Username&passwd=Password&charset=UTF8')
> 
> And everything should be done in driver but not interface?
> 

Ah, the question has changed to "Shall we need a uniform DSN?"

Shall we define some uniform dsn fields, and then drivers decide the others.

user:passwd@host:port/?charset=utf8&k1=v1&k2=v2

Right?

Or there's no need to have a standard like this. 
All decided by the driver.

Eh... I must talk about this with ziutek.

Thanks all. Sorry to bother you!

[ziutek, 2012-02-29 10:17:06]

On 2012/02/28 22:39:04, bradfitz wrote:
> I think this is something drivers should provide.  You can pack any form of
> arguments into the driver.Open(name string).  That name string can be any
> format.
> 
> In short, I don't think this requires any database/sql* changes.

I don't think that pass huge of initialization commands over name string is good
idea. This is definitely database/sql issue resulting from the concept of
implicit connections establishing.

I encountered this problem a long time ago (when creating mymysql autoreconnect
interface) and as a result Register method appeared (with it you can register
any number of commands for connection initialization).

[ziutek, 2012-02-29 10:36:20]

On 2012/02/29 02:50:16, MC.Spring wrote:
> I would prefer to use Query String model, suck as: 
> 
> db, err := sql.Open('mymysql', 'user=Username&passwd=Password&charset=UTF8')

Passwords can contain any char (=?/& 写 to) so with your model you need some
escape function for it. In mymysql model password is last part of name string so
it can be any string.