code review 4973055: exp/sql{,/driver}: new database packages

exp/sql{,/driver}: new database packages

[bradfitz, 2011-09-02 17:04:13]

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

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

[bradfitz, 2011-09-02 17:04:26]

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

Please take another look.

[bradfitz, 2011-09-02 17:05:39]

Docs at:

     http://p.danga.com:6060/pkg/exp/db/
     http://p.danga.com:6060/pkg/exp/db/dbimpl/

Goals of the db and db/dbimpl packages:

* Provide a generic database API for a variety of SQL or SQL-like
  databases.  There currently exist Go libraries for SQLite, MySQL,
  and Postgres, but all with a very different feel, and often
  a non-Go-like feel.

* Feel like Go.

* Care mostly about the common cases. Common SQL should be portable.
  SQL edge cases or db-specific extensions can be detected and
  conditionally used by the application.  It is a non-goal to care
  about every particular db's extension or quirk.

* Separate out the basic implementation of a database driver
  (implementing the db/dbimpl interfaces) vs the implementation
  of all the user-level types and convenience methods.
  In a nutshell:

  User Code ---> db package (concrete types) ---> db/dbimpl (interfaces)
  Database Driver -> db (to register) + dbimpl (implement interfaces)

* To type casting/conversions consistently between all drivers. To
  achieve this, most of the type conversions are done in the db
  package, not in each driver.  The drivers then only have to deal
  with a smaller set of types.

* Be flexible with type conversions, but be paranoid about silent
  truncation or other loss of precision.

* Handle concurrency well.  Users shouldn't need to care about the
  database's per-connection thread safety issues (or lack thereof),
  and shouldn't have to maintain their own free pools of connections.
  The 'db' package should deal with that bookkeeping as needed.  Given
  a *db.DB, it should be possible to share that instance between
  multiple goroutines, without any extra synchronization.

* Push complexity, where necessary, down into the db+dbimpl packages,
  rather than exposing it to users. Said otherwise, the db package
  should expose an ideal database that's not finnicky about how it's
  accessed, even if that's not true.

* Provide optional interfaces in dbimpl for drivers to implement
  for special cases or fastpaths.  But the only party that knows about
  those is the 'db' package.  To user code, some stuff just might start
  working or start working slightly faster.

On Fri, Sep 2, 2011 at 10:04 AM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
>
http://codereview.appspot.com/**4973055/<http://codereview.appspot.com/4973055/>
>

[bradfitz, 2011-09-02 17:07:10]

Lost in the original CL description after a server error:

This was designed with Russ a couple months ago.

A couple things changed slightly found during implementation, but it's very
close to the original design.


On Fri, Sep 2, 2011 at 10:05 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Docs at:
>
>      http://p.danga.com:6060/pkg/exp/db/
>      http://p.danga.com:6060/pkg/exp/db/dbimpl/
>
> Goals of the db and db/dbimpl packages:
>
> * Provide a generic database API for a variety of SQL or SQL-like
>   databases.  There currently exist Go libraries for SQLite, MySQL,
>   and Postgres, but all with a very different feel, and often
>   a non-Go-like feel.
>
> * Feel like Go.
>
> * Care mostly about the common cases. Common SQL should be portable.
>   SQL edge cases or db-specific extensions can be detected and
>   conditionally used by the application.  It is a non-goal to care
>   about every particular db's extension or quirk.
>
> * Separate out the basic implementation of a database driver
>   (implementing the db/dbimpl interfaces) vs the implementation
>   of all the user-level types and convenience methods.
>   In a nutshell:
>
>   User Code ---> db package (concrete types) ---> db/dbimpl (interfaces)
>   Database Driver -> db (to register) + dbimpl (implement interfaces)
>
> * To type casting/conversions consistently between all drivers. To
>   achieve this, most of the type conversions are done in the db
>   package, not in each driver.  The drivers then only have to deal
>   with a smaller set of types.
>
> * Be flexible with type conversions, but be paranoid about silent
>   truncation or other loss of precision.
>
> * Handle concurrency well.  Users shouldn't need to care about the
>   database's per-connection thread safety issues (or lack thereof),
>   and shouldn't have to maintain their own free pools of connections.
>   The 'db' package should deal with that bookkeeping as needed.  Given
>   a *db.DB, it should be possible to share that instance between
>   multiple goroutines, without any extra synchronization.
>
> * Push complexity, where necessary, down into the db+dbimpl packages,
>   rather than exposing it to users. Said otherwise, the db package
>   should expose an ideal database that's not finnicky about how it's
>   accessed, even if that's not true.
>
> * Provide optional interfaces in dbimpl for drivers to implement
>   for special cases or fastpaths.  But the only party that knows about
>   those is the 'db' package.  To user code, some stuff just might start
>   working or start working slightly faster.
>
> On Fri, Sep 2, 2011 at 10:04 AM, <bradfitz@golang.org> wrote:
>
>> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>>
>> Please take another look.
>>
>>
>>
>>
http://codereview.appspot.com/**4973055/<http://codereview.appspot.com/4973055/>
>>
>
>

[gustavo_niemeyer.net, 2011-09-02 17:23:23]

Hey Brad,

> * Provide a generic database API for a variety of SQL or SQL-like
>   databases.  There currently exist Go libraries for SQLite, MySQL,
>   and Postgres, but all with a very different feel, and often
>   a non-Go-like feel.

Just one minor point from the previous conversation on the topic:
given that there are so many databases out there that do not suit this
API design, and given the examples you use above, should this be more
properly called as sqldb?

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[bradfitz, 2011-09-02 17:29:44]

On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> Hey Brad,
>
> > * Provide a generic database API for a variety of SQL or SQL-like
> >   databases.  There currently exist Go libraries for SQLite, MySQL,
> >   and Postgres, but all with a very different feel, and often
> >   a non-Go-like feel.
>
> Just one minor point from the previous conversation on the topic:
> given that there are so many databases out there that do not suit this
> API design, and given the examples you use above, should this be more
> properly called as sqldb?
>

I'd rather not but I'm not going to fight either way.

Renaming it is easy.  Note that it's in exp/ right now, too.  I'd rather
focus on the code first.

[rsc, 2011-09-02 17:31:34]

Is there any hope of a more general package called db?
If not, then db is fine.  I think there's a strong argument
to be made that those things are not databases.  For
example, people sometimes call bigtable a database,
but it's not.

Russ

[gustavo_niemeyer.net, 2011-09-02 17:46:15]

> Is there any hope of a more general package called db?

Having nothing more general is not good reasoning for calling a
well-bounded concept generically.  I could argue about databases that
do not follow this API, but it's way easier to just point out that the
API was clearly designed with SQL databases in mind, so why not naming
it as such?

Besides, "db" is a fantastic variable name in that space. It'd be a
shame to kill it.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[bradfitz, 2011-09-02 17:52:24]

On Fri, Sep 2, 2011 at 10:45 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> > Is there any hope of a more general package called db?
>
> Having nothing more general is not good reasoning for calling a
> well-bounded concept generically.  I could argue about databases that
> do not follow this API, but it's way easier to just point out that the
> API was clearly designed with SQL databases in mind, so why not naming
> it as such?
>
> Besides, "db" is a fantastic variable name in that space. It'd be a
> shame to kill it.
>

If anything I'd buy that argument most strongly.

[borman, 2011-09-02 17:52:35]

I agree.  I think something like sql, sqldb, or dbsql.

This CL makes me long for a go package for Berkeley DB (sleepycat)...
 Fortunately I don't need it for my project since cgo is not (yet?) an
option for me.

On Fri, Sep 2, 2011 at 10:45 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> > Is there any hope of a more general package called db?
>
> Having nothing more general is not good reasoning for calling a
> well-bounded concept generically.  I could argue about databases that
> do not follow this API, but it's way easier to just point out that the
> API was clearly designed with SQL databases in mind, so why not naming
> it as such?
>
> Besides, "db" is a fantastic variable name in that space. It'd be a
> shame to kill it.
>
> --
> Gustavo Niemeyer
> http://niemeyer.net
> http://niemeyer.net/plus
> http://niemeyer.net/twitter
> http://niemeyer.net/blog
>
> -- I never filed a patent.
>

[bradfitz, 2011-09-02 20:00:24]

On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> Hey Brad,
>
> > * Provide a generic database API for a variety of SQL or SQL-like
> >   databases.  There currently exist Go libraries for SQLite, MySQL,
> >   and Postgres, but all with a very different feel, and often
> >   a non-Go-like feel.
>
> Just one minor point from the previous conversation on the topic:
> given that there are so many databases out there that do not suit this
> API design, and given the examples you use above, should this be more
> properly called as sqldb?


Latest thinking:

sql
sql/driver (maybe; better than sql/sqlimpl)

Then it's *sql.DB and driver.Conn, etc.

[gustavo_niemeyer.net, 2011-09-03 00:08:10]

> Latest thinking:
> sql
> sql/driver (maybe; better than sql/sqlimpl)
> Then it's *sql.DB and driver.Conn, etc.

+1

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[dave_cheney.net, 2011-09-03 00:15:43]

+1

Sent from my iPad

On 03/09/2011, at 6:00, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net>
wrote:
> Hey Brad,
> 
> > * Provide a generic database API for a variety of SQL or SQL-like
> >   databases.  There currently exist Go libraries for SQLite, MySQL,
> >   and Postgres, but all with a very different feel, and often
> >   a non-Go-like feel.
> 
> Just one minor point from the previous conversation on the topic:
> given that there are so many databases out there that do not suit this
> API design, and given the examples you use above, should this be more
> properly called as sqldb?
> 
> Latest thinking:
> 
> sql
> sql/driver (maybe; better than sql/sqlimpl)
> 
> Then it's *sql.DB and driver.Conn, etc.

[kevlar, 2011-09-03 01:26:10]

I like the idea of this.  While I don't think it's necessarily incompatible with
some other interfaces to allow it to do document-oriented and object-oriented
databases, it's definitely SQL-driven now.

For this, I like the sql and sql/driver names, so I use those in my comments as
if that's what they were named (because I can't seem to type dbimpl the first
time without a typo).

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go
File src/pkg/exp/db/db.go (right):

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode18
src/pkg/exp/db/db.go:18: // databases.
I would put an example of how this might be used given a fictitions "foosql"
package, given that the underscore-imports can take some getting-used-to:

import (
  "exp/sql"
  _ "somewhere.online.com/foosql"
)

...
etc

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode34
src/pkg/exp/db/db.go:34: // a duplicate name.
I would mention that this should be called in the init() method of a particular
SQL driver's package.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode61
src/pkg/exp/db/db.go:61: Ok     bool // Ok is true if the String is not NULL
I would call this NotNull?

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode276
src/pkg/exp/db/db.go:276: type connStmt struct {
This name (and its associated function) read awkwardly for me.  I don't know if
calling it "prepared" or something would make matters better or worse.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode297
src/pkg/exp/db/db.go:297: func (s *Stmt) Exec(args ...interface{}) (Result,
os.Error) {
Exec is envious of Query's comment.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode346
src/pkg/exp/db/db.go:346: if _, match = s.db.connIfFree(cs.ci); match {
Is there no clean way to do this without locks?  Without connIfFree, you could
probably make do with a buffered channel from which you popped free connections
and to which you pushed ones with which you are done.  connIfFree would have to
pop all of them and push them back, which might be prohibitive.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode399
src/pkg/exp/db/db.go:399: // always non-nil.
Provide or refer to the example of chaining this with Scan?

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode497
src/pkg/exp/db/db.go:497: }
The functional programmer in me wants:

func (rs *Rows) Each(func(...interface{})) os.Error {}

which should be possible by reflecting over the arguments and making new Values
for each one?  Overkill?  Part of me thinks that the overhead of the initial
reflection wouldn't be too bad when compared with a large dataset... unless I'm
overestimating how much of the reflection would be precomputed and how much
would happen every iteration.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode535
src/pkg/exp/db/db.go:535: AutoIncrementId() (int64, os.Error)
LastInsertId?

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode536
src/pkg/exp/db/db.go:536: RowsAffected() (int64, os.Error)
AffectedRows?

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/fakedb_test.go
File src/pkg/exp/db/fakedb_test.go (right):

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/fakedb_test.go...
src/pkg/exp/db/fakedb_test.go:32: // fakeDriver is a fake database that
implements Go's dbimpl.Driver
lol.  most. elaborate. test. case. ever. love it.  with a bit of a parser, you
could almost extend this to be a basic SQL engine and include it in the stdlib.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/fakedb_test.go...
src/pkg/exp/db/fakedb_test.go:510: }
This file embodies lots of the reasons that I love Go.

[nigeltao, 2011-09-03 01:49:16]

On 3 September 2011 03:04,  <bradfitz@golang.org> wrote:
> Description:
> exp/db{,/dbimpl}: new database packages
>
> Please review this at http://codereview.appspot.com/4973055/

Here's a bunch of half-baked thoughts. Other than a comment on
licensing, I've based it more on the API docs than the actual
implementation (which I've only skimmed).

The code should be BSD-style license, not Apache.

It seems odd that most of package db's types are structs (with no
exported fields) and not interfaces. Especially since DB, Stmt and Tx
all share Exec, Query and QueryRow methods.

I would rename Rows to Iter or Iterator. Also, I'd paint the bike shed
Txn instead of Tx, and s/dest/dst/g.

If I squint, it might be possible for the App Engine datastore to fit
these interfaces (it would be a GQL database instead of an SQL
database). AE "row" keys are a structured type and not a string or
int, but maybe it will still work if a datastore.Key implements
ScannerInto. It's not as if Row has a Name() string method.

Transactions in the App Engine datastore are represented as a
retry-able func, instead of explicit Begin/Commit/Rollback calls. I'm
not 100% sure that the functional approach is best, but it is
something a little higher level that's not possible in C/C++ style
APIs

SQL Noob question #0: if I understand your API, I can't run a Stmt in
a Tx, I have to parse it for every transaction by calling Tx.Prepare.
Should Exec and Query take a Stmt instead of a string as their first
arg?

SQL Noob question #1: do we need to distinguish between Exec and
Query? Can we just run statements, some of which have zero results?

Tx.Exec has a different return type than DB.Exec or Stmt.Exec.

Finally, the Rows interface and usage are:
----
// Yeah, it's actually a struct, but for argument's sake...
type Rows interface {
  Close() os.Error
  Error() os.Error
  Next() bool
  Scan(dest ...interface{}) os.Error
}
----
rows, err := db.Query("SELECT ...")
...
for rows.Next() {
    var id int
    var name string
    err = rows.Scan(&id, &name)
    ...
}
err = rows.Error()
----
First, is there a need for Rows to have a separate Error and Close methods?

Finally, I'll compare these with iterating over the AE datastore, and
iterating over a LevelDB Go port that I'm working on (LevelDB is a
key/value store). I'm not saying that these other designs are
necessarily better, and neither of them are set in stone. Still, their
experience may be instructive in exploring the design space.

The App Engine iterator interface and usage currently are:
----
type Iterator interface {
  Next(dst interface{}) (*Key, os.Error)
}
----
iter := query.Run(context)
for {
  var x Widget
  key, err := iter.Next(&x)
  if err == datastore.Done {
    return nil
  }
  if err != nil {
    return err
  }
  doSomethingWith(key, &x)
}
----

The LevelDB iterator interface and usage currently are:
----
type Iterator interface {
  Next() bool
  Key() []byte
  Value() []byte
  Close() os.Error
}
----
iter := db.Seek(key)
for iter.Next() {
  doSomethingWith(iter.Key(), iter.Value())
}
return iter.Close()
----

For AppEngine, I didn't think of having separate Next and Scan
methods. That design is growing on me.

For LevelDB, those that create Iterators (via calling DB.Seek) are
required to Close them, to avoid resource leaks.

For both, neither Query.Run and DB.Seek returns an error. If something
bad occurs, they returns an errorIterator instead: an implementation
of the Iterator interface that has no results but that error.

[bradfitz, 2011-09-03 02:02:00]

(replies to some comments below; others later)

On Fri, Sep 2, 2011 at 6:49 PM, Nigel Tao <nigeltao@golang.org> wrote:

> On 3 September 2011 03:04,  <bradfitz@golang.org> wrote:
> > Description:
> > exp/db{,/dbimpl}: new database packages
> >
> > Please review this at http://codereview.appspot.com/4973055/
>
> Here's a bunch of half-baked thoughts. Other than a comment on
> licensing, I've based it more on the API docs than the actual
> implementation (which I've only skimmed).
>
> The code should be BSD-style license, not Apache.
>

whoops. I was prototyping this elsewhere.  will fix.


> It seems odd that most of package db's types are structs (with no
> exported fields) and not interfaces. Especially since DB, Stmt and Tx
> all share Exec, Query and QueryRow methods.
>

by design.  pkg dbimpl is interfaces and pkg db is structs.


> Transactions in the App Engine datastore are represented as a
> retry-able func, instead of explicit Begin/Commit/Rollback calls. I'm
> not 100% sure that the functional approach is best, but it is
> something a little higher level that's not possible in C/C++ style
> APIs
>

That could be added on top, too.  I'm torn.  I love abusing functional
style, but I'm also not a huge fan of that API in App Engine.  It leads to
people doing non-database things in their transaction functions.


> SQL Noob question #0: if I understand your API, I can't run a Stmt in
> a Tx, I have to parse it for every transaction by calling Tx.Prepare.
> Should Exec and Query take a Stmt instead of a string as their first
> arg?
>

Tx stuff isn't implemented and is mostly from old notes.  it's probably
behind.

but you talking about db or dbimpl?


> SQL Noob question #1: do we need to distinguish between Exec and
> Query? Can we just run statements, some of which have zero results?
>

it lets drivers do different things if desired to not send results back over
the network if the caller's intentions is to discard them anyway.


> Tx.Exec has a different return type than DB.Exec or Stmt.Exec.
>

originally in the notes exec was just returning  os.Error, but then during
implementation I found a result was desired.  just didn't get around to
implementing Tx yet.

[nigeltao, 2011-09-03 02:12:52]

On 3 September 2011 12:01, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> It seems odd that most of package db's types are structs (with no
>> exported fields) and not interfaces. Especially since DB, Stmt and Tx
>> all share Exec, Query and QueryRow methods.
>
> by design.  pkg dbimpl is interfaces and pkg db is structs.

I know that it's deliberate. It still feels wrong to me. I feel like I
should be able to make a fake DB type without needing to go through
db/dbimpl.


> Tx stuff isn't implemented and is mostly from old notes.  it's probably
> behind.
> but you talking about db or dbimpl?

I've looked mostly at db, since that's the consumer-facing API.


>> SQL Noob question #1: do we need to distinguish between Exec and
>> Query? Can we just run statements, some of which have zero results?
>
> it lets drivers do different things if desired to not send results back over
> the network if the caller's intentions is to discard them anyway.

And that's not representable by the query language? Like I said, I'm a
SQL Noob...

[nigeltao, 2011-09-03 02:37:43]

On 3 September 2011 12:12, Nigel Tao <nigeltao@golang.org> wrote:
> I know that it's deliberate. It still feels wrong to me. I feel like I
> should be able to make a fake DB type without needing to go through
> db/dbimpl.

A stronger statement would be that, with this design, I can't
implement a DB without going through db/dbimpl. If I wanted to make
the App Engine datastore fit this model, I wouldn't be able to make a
type assertion that the db.Row that I get from the AppEngine driver
has a Key method as well as a Scan method.

Oversimplifying the argument: it's weird that the 'implementation'
package has the interfaces but the public-facing Application
Programming Interface package has the structs. Although that's
probably a better argument for renaming s/dbimpl/driver/.

[bradfitz, 2011-09-03 03:17:22]

On Fri, Sep 2, 2011 at 7:37 PM, Nigel Tao <nigeltao@golang.org> wrote:

> On 3 September 2011 12:12, Nigel Tao <nigeltao@golang.org> wrote:
> > I know that it's deliberate. It still feels wrong to me. I feel like I
> > should be able to make a fake DB type without needing to go through
> > db/dbimpl.
>
> A stronger statement would be that, with this design, I can't
> implement a DB without going through db/dbimpl. If I wanted to make
> the App Engine datastore fit this model, I wouldn't be able to make a
> type assertion that the db.Row that I get from the AppEngine driver
> has a Key method as well as a Scan method.
>

The point of making them structs was so we had a way to add convenience
wrapper stuff like QueryRow:

http://p.danga.com:6060/src/pkg/exp/db/db.go?s=5797:5859#L217

You wouldn't want QueryRow part of the interface, since it's redundant with
Query.  But we want stuff like that (and probably more over time) in package
db because it's super nice.

If you want to do driver-specific stuff, use the Driver() method and
type-assert on that.

Would you be happy if the db package had an OpenDriver(impl driver.Driver)?
 Then you don't need to register a driver name at least?

[dvyukov, 2011-09-03 10:17:53]

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go
File src/pkg/exp/db/db.go (right):

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode35
src/pkg/exp/db/db.go:35: func Register(name string, driver dbimpl.Driver) {
If it's not intended for end user use, shouldn't it go to dbimpl (with the
associated driver query function)?

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode83
src/pkg/exp/db/db.go:83: var ErrNoRows = os.NewError("db: no rows in result
set")
Don't we want to prevent users from assigning to it (like they do with
os.Stdout)?

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode346
src/pkg/exp/db/db.go:346: if _, match = s.db.connIfFree(cs.ci); match {
What is the expected number of s.css and db.freeConn?

[kardia, 2011-09-03 20:19:41]

Your Rows type is more of a Cursor or Iter then a Rows struct.

For most SQL applications, I tend to buffer my result set rather then 
iterating it directly with an open connection.  Thus, Stmt.Fill() returns 
Table with all data in it.  I can then return and close the connection and 
put my logic elsewhere.

I like the driver register design.

bikeshed: the api is full of abbreviations.  I can type Statement faster 
then Stmt.  I'd prefer if they were full words.

[rsc, 2011-09-04 16:45:46]

On Fri, Sep 2, 2011 at 22:37, Nigel Tao <nigeltao@golang.org> wrote:
> A stronger statement would be that, with this design, I can't
> implement a DB without going through db/dbimpl.

That is by design.  There's a lot about data marshaling and
general API consistency that is subtle and that we want
implemented in exactly one place.  sql/driver defines what
the database has to supply as basic operations, and then sql
builds the nicer API on top of that.  In this way sql/driver is
kind of like io.Reader and sql is more like io.ReadFull.

Russ

[fw, 2011-09-05 04:58:12]

* Brad Fitzpatrick:

> * To type casting/conversions consistently between all drivers. To
>   achieve this, most of the type conversions are done in the db
>   package, not in each driver.  The drivers then only have to deal
>   with a smaller set of types.

Both PostgreSQL and SQLite need to distinguish between strings
(encoded in UTF-8) and blobs.

> * Be flexible with type conversions, but be paranoid about silent
>   truncation or other loss of precision.

There's no good match in Go for the NUMERIC type right now.

> * Handle concurrency well.  Users shouldn't need to care about the
>   database's per-connection thread safety issues (or lack thereof),
>   and shouldn't have to maintain their own free pools of connections.
>   The 'db' package should deal with that bookkeeping as needed.  Given
>   a *db.DB, it should be possible to share that instance between
>   multiple goroutines, without any extra synchronization.

I think there's still a need to obtain a specific connection, without
starting a transaction: temporary tables are tied to a specific
connection, and some database statements cannot run in a transaction.

> * Push complexity, where necessary, down into the db+dbimpl packages,
>   rather than exposing it to users. Said otherwise, the db package
>   should expose an ideal database that's not finnicky about how it's
>   accessed, even if that's not true.

Some things are impossible to hide, like serialization failures.

There's currently no way to check if an error will likely go away when
a transaction is retried.

[nigeltao, 2011-09-05 11:32:17]

On 3 September 2011 13:17, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> The point of making them structs was so we had a way to add convenience
> wrapper stuff like QueryRow:
> http://p.danga.com:6060/src/pkg/exp/db/db.go?s=5797:5859#L217

The counterpoint is that if db.Row is a struct, then even if foo.Row
embeds a db.Row, I can't convert from the latter to the former (even
if I have a Driver instance). You could if both were interfaces.

I still think that you can have structs and interfaces in the
sql/driver package that handles the subtleties for driver
implementations, but end-users of the sql package can program to
interfaces.

But this isn't my package. I'll get out of the kitchen; there's more
than enough cooks already.

[nigeltao, 2011-09-07 01:47:33]

On 3 September 2011 11:49, Nigel Tao <nigeltao@golang.org> wrote:
> // Yeah, it's actually a struct, but for argument's sake...
> type Rows interface {
>  Close() os.Error
>  Error() os.Error
>  Next() bool
>  Scan(dest ...interface{}) os.Error
> }

On a tangent, but... some iterators support backwards iteration, but
there are two semantic models for this: whether iterators are at a
row, or are between rows.

Suppose my query returns three rows (A, B, C), and on my iterator (or
what you've named Rows) I call Next, Next, Prev, Scan.

With the "at" model, Scan returns the data where the iterator is at:
The iterator starts to the left of A.
Next moves it to A.
Next moves it to B.
Prev moves it to A.
Scan returns A.

With the "between" model, Scan returns the data most recently walked over:
The iterator starts at the "$" in "$ABC".
Next moves it to "A$BC", having walked over A.
Next moves it to "AB$C", having walked over B.
Prev moves it to "A$BC", having walked over B.
Scan returns B.

Which model seems more intuitive to golang-dev? Sure, this question is
hypothetical, since the proposed db or sql package doesn't provide
backwards iteration, but it would be nice if the iteration model was
consistent across other Go database packages.



Getting back to the API design in the original CL, an alternative to a

Next() bool

method is to have two methods:

Done() bool
Next()

and the typical loop changes from:

for rows.Next() {
  // do stuff with rows.Scan.
}

to

for ; !rows.Done(); rows.Next() {
  // do stuff with rows.Scan.
}

Note that the two differ in that the first Next call happens before
the first Scan call in the former but after in the latter.

Does the answer to the "are iterators at or between" question
influence the answer to the "do iterators loop for Next or for
;!Done;Next" question?

[rsc, 2011-09-07 02:05:03]

I think you are reasoning backwards.
The goal is to have a nice, easy-to-use API, hence

for rows.Next() {
    rows.Scan...
}

We can assign whatever semantics to Next
that we like as long as it produces that pattern.

If you need backward iteration, I would say that
it should be asked for explicitly as part of the
setup rather than by calling different methods
during the traversal itself (like SQL's 'DESC').
If the rows come out backwards, that's fine;
the loop shouldn't change.

Russ

[nigeltao, 2011-09-07 02:48:23]

On 7 September 2011 12:04, Russ Cox <rsc@golang.org> wrote:
> The goal is to have a nice, easy-to-use API, hence
>
> for rows.Next() {
>    rows.Scan...
> }
>
> We can assign whatever semantics to Next
> that we like as long as it produces that pattern.

I agree that this is a nice API. Either "at" or "between" iterators
will work with this API.

I would like leveldb-go to provide a similarly nice API, but leveldb
supports backwards iteration (because it's needed by e.g. Chromium's
IndexedDB support), and it was unclear to me how Prev should work.

However, I had not thought of specifying the iteration direction at
setup instead of traversal. That might simply be the nicest solution.

[bradfitz, 2011-09-16 00:04:42]

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go
File src/pkg/exp/db/db.go (right):

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode61
src/pkg/exp/db/db.go:61: Ok     bool // Ok is true if the String is not NULL
On 2011/09/03 01:26:10, kevlar wrote:
> I would call this NotNull?

Done, but it feels a bit funny in expressions like:

if !s.NotNull {

(double negative)

We might want to name it "Null".

I'm happy with Ok, Null, or NotNull.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode83
src/pkg/exp/db/db.go:83: var ErrNoRows = os.NewError("db: no rows in result
set")
On 2011/09/03 10:17:54, dvyukov wrote:
> Don't we want to prevent users from assigning to it (like they do with
> os.Stdout)?

I'm not going to fight the language in this CL.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode297
src/pkg/exp/db/db.go:297: func (s *Stmt) Exec(args ...interface{}) (Result,
os.Error) {
On 2011/09/03 01:26:10, kevlar wrote:
> Exec is envious of Query's comment.

Done.

http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode399
src/pkg/exp/db/db.go:399: // always non-nil.
On 2011/09/03 01:26:10, kevlar wrote:
> Provide or refer to the example of chaining this with Scan?

Done.

[bradfitz, 2011-09-16 00:04:44]

Hello golang-dev@googlegroups.com, gustavo@niemeyer.net, rsc@golang.org,
borman@google.com, dave@cheney.net, kevlar@google.com, nigeltao@golang.org,
dvyukov@google.com, kardianos@gmail.com, fw@deneb.enyo.de (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-09-16 00:12:07]

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go
File src/pkg/exp/sql/sql.go (right):

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:45: // NullableString is type representing a string which
may be
s/which/that/

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is not NULL
i dislike negative names. you always end up negating them again in expressions.

it's negative so the zero type is an empty string? how about Valid?

[bradfitz, 2011-09-16 00:14:11]

On Thu, Sep 15, 2011 at 5:12 PM, <r@golang.org> wrote:

>
> http://codereview.appspot.com/**4973055/diff/28001/src/pkg/**
>
exp/sql/sql.go#newcode61<http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newcode61>
> src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is
> not NULL
> i dislike negative names. you always end up negating them again in
> expressions.
>

agree.  I already did that in the test code and it looks gross.


> it's negative so the zero type is an empty string? how about Valid?


Isn't Valid just a long way of spelling Ok?  (which is how I had it in v0,
and seems like the Go word....)

[r2, 2011-09-16 00:16:07]

On Sep 15, 2011, at 5:14 PM, Brad Fitzpatrick wrote:

> On Thu, Sep 15, 2011 at 5:12 PM, <r@golang.org> wrote:
> 
>
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
> src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is
> not NULL
> i dislike negative names. you always end up negating them again in
> expressions.
> 
> agree.  I already did that in the test code and it looks gross.
>  
> it's negative so the zero type is an empty string? how about Valid?
> 
> Isn't Valid just a long way of spelling Ok?  (which is how I had it in v0, and
seems like the Go word....)

I prefer Valid. We use it in reflect for essentially the same idea.  OK is the
word (not Ok) and anyway we use it as a variable name for various things. This
is asking if the string is valid, which is a narrower meaning.

Wow, today really is all about how to bikeshed. We should start a discussion
group about the best way to bikeshed.

-rob

[bradfitz, 2011-09-16 00:23:43]

Valid it is.

There are a few thousand other lines to review too.


On Thu, Sep 15, 2011 at 5:15 PM, Rob 'Commander' Pike <r@google.com> wrote:

>
> On Sep 15, 2011, at 5:14 PM, Brad Fitzpatrick wrote:
>
> > On Thu, Sep 15, 2011 at 5:12 PM, <r@golang.org> wrote:
> >
> >
>
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
> > src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is
> > not NULL
> > i dislike negative names. you always end up negating them again in
> > expressions.
> >
> > agree.  I already did that in the test code and it looks gross.
> >
> > it's negative so the zero type is an empty string? how about Valid?
> >
> > Isn't Valid just a long way of spelling Ok?  (which is how I had it in
> v0, and seems like the Go word....)
>
> I prefer Valid. We use it in reflect for essentially the same idea.  OK is
> the word (not Ok) and anyway we use it as a variable name for various
> things. This is asking if the string is valid, which is a narrower meaning.
>
> Wow, today really is all about how to bikeshed. We should start a
> discussion group about the best way to bikeshed.
>
> -rob
>
>

[r2, 2011-09-16 01:45:43]

I don't know SQL worth a damn but I'll look it over for second-order stuff.

[crawshaw2, 2011-09-16 04:16:57]

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go
File src/pkg/exp/sql/sql.go (right):

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:92: freeConn []driver.Conn
So you are maintaining an underlying connection pool and not exposing the
individual connections to the client. How does this work with transactions,
which are only valid on a single connection?

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:235: func (db *DB) Begin() (*Tx, os.Error) {
There are different types of transactions (e.g. deferred, immediate, exclusive).
How about Begin(arg string)?

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:255: func (tx *Tx) Rollback() os.Error {
When a DB supports nested transactions, you can do partial rollbacks to named
savepoints:

http://www.sqlite.org/lang_savepoint.html

(This is common with PostgreSQL, Oracle, MSSQL, etc.)

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:462: func (rs *Rows) Next() bool {
Probably best left for some later version of the API, but fancy DBs have
different kinds of cursors.

The classic also has Prev, First, Last, which you'll find in the original JDBC
API. Another common cursor power is Count.

[rsc, 2011-09-16 15:17:57]

Only looking at public API.
More comments in driver would help.

My main concern right now is that the value 
conversion code seems to be scattered, possibly
too general.  I'd like to make sure we end up
with something where you don't have different
drivers supporting different kinds of values.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/convert.go
File src/pkg/exp/sql/convert.go (right):

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/convert.go#n...
src/pkg/exp/sql/convert.go:4: Licensed under the Apache License, Version 2.0
(the "License");
Copyright notices are still wrong.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
File src/pkg/exp/sql/driver/driver.go (right):

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:38: // The dsn parameter, the Data Source Name,
contains a
// The name is a string in a driver-specific format.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:44: Open(dsn string) (Conn, os.Error)
s/dsn/name/

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:50: // If not implemented by a Driver, the sql
package's DB.Exec method
the implicit noun here sounds like it is 'the sql package's DB.Exec method'.

// If a Driver does not implement Execer, the sql package's DB.Exec method

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:55: // If not implemented by a Conn, the db
package's DB.Exec will first
// If a Conn does not implement Execer, the db package's DB.Exec will ...

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:62: type Conn interface {
more comments...

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:90: // ColumnConverter may be optionally
implemented by Stmt to signal
// If a Stmt implements ColumnConverter, the sql package calls
// its ColumnConverter method to perform type conversions.

or whatever, but is this really necessary?
Part of the rationale for having a central sql package
is so that the conversions are in one place.
I might not be as worried if you define what ValueConverter
must be able to do.

Also, is it necessary to have two different methods?
You could instead have

type Converter interface {
    Convert(index int, v interface{}) (interface{}, os.Error)
}

Maybe leave this out for round 1.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:93: ColumnCoverter(idx int) ValueConverter
Converter is spelled wrong

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:125: var DDLSuccess Result = ddlSuccess{}
?

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go
File src/pkg/exp/sql/sql.go (right):

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:32: // Register makes a database driver available by the
provided name.
RegisterDriver please.
It's not supposed to be called from user code
so a more precise name is fine (and makes it less
likely that people will think they need to use it).

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:39: if _, dup := drivers[name]; dup {
or if drivers[name] != nil

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:80: // ErrNoRows is returned by Scan when QueryRow
doesn't return a
This is kind of awkward.  I think it's okay to break the 
leading name rule here to be less awkward.  (Using complete
sentences makes this possible.)

// When QueryRow finds no rows, it returns a placeholder *Row;
// that row's Scan method returns ErrNoRows.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:89: dsn    string
s/dsn/name/

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:99: // Most users will open a database via a
driver-specific connection
Why?

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:101: func Open(driverName, dataSourceName string) (*DB,
os.Error) {
func Open(driver, name string) (*DB, os.Error) {

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:103: if !ok {
or driver == nil

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:156: func (db *DB) Prepare(query string) (*Stmt,
os.Error) {
Comments on public API functions?

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:240: // This is non-portable and should only be used when
Delete this sentence?  It's not clear what non-portable means
and "when needed" is of course a matter of judgment.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:297: // Exec executes a prepared statement with the
provided placeholder arguments
// Exec executes a prepared statement with the given arguments and
// returns a Result summarizing the effect of the statement.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:375: // Query executes a prepared statement with the
provided placeholder arguments
// Query executes a prepared query statement with the given arguments
// and returns the query results as a *Rows.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:399: // QueryRow wraps Query, for use in selecting a
single row. Any errors
// QueryRow executes a prepared query statement with the given arguments.
// If an error occurs during the execution of the statement, that error will
// be returned by a call to Scan on the returned *Row, which is always non-nil.
// If the query selects no rows, the *Row's Scan will return ErrNoRows.
// Otherwise, the *Row's Scan scans the first selected row and discards
// the rest.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:459: // Next advances the Rows' cursor (which starts
before the first
' without s is only for plurals.  Rows is a singular here.
but we can avoid the term cursor entirely, to avoid
people thinking this is a JDBC cursor.

// Next prepares the next result row for reading with the Scan method. 
// It returns true on success, false if there is no next result row.
// Every call to Scan, even the first one, must be preceded by a call to Next.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:484: // Scan copies the columns in the current row into
variables referened in dest.
into the values pointed at by dest.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:520: // Row is the result of calling QueryRow to select a
single row. If no
I think you can drop the second sentence, leaving it for QueryRow

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:529: // Scan copies the columns from the matched row into
the variables
// Scan copies the columns from the matched row into the values
// pointed at by dest.  If more than one row matches the query,
// Scan uses the first row and discards the rest.  If no row matches
// the query, Scan returns ErrNoRows.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:542: type Result interface {
// A Result summarizes an executed SQL command.

[bradfitz, 2011-09-29 01:08:55]

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
File src/pkg/exp/sql/driver/driver.go (right):

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:38: // The dsn parameter, the Data Source Name,
contains a
On 2011/09/16 15:17:58, rsc wrote:
> // The name is a string in a driver-specific format.

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:44: Open(dsn string) (Conn, os.Error)
On 2011/09/16 15:17:58, rsc wrote:
> s/dsn/name/

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:50: // If not implemented by a Driver, the sql
package's DB.Exec method
On 2011/09/16 15:17:58, rsc wrote:
> the implicit noun here sounds like it is 'the sql package's DB.Exec method'.
> 
> // If a Driver does not implement Execer, the sql package's DB.Exec method

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:55: // If not implemented by a Conn, the db
package's DB.Exec will first
On 2011/09/16 15:17:58, rsc wrote:
> // If a Conn does not implement Execer, the db package's DB.Exec will ...

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:62: type Conn interface {
On 2011/09/16 15:17:58, rsc wrote:
> more comments...

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:90: // ColumnConverter may be optionally
implemented by Stmt to signal
On 2011/09/16 15:17:58, rsc wrote:
> // If a Stmt implements ColumnConverter, the sql package calls
> // its ColumnConverter method to perform type conversions.
> 
> or whatever, but is this really necessary?
> Part of the rationale for having a central sql package
> is so that the conversions are in one place.
> I might not be as worried if you define what ValueConverter
> must be able to do.
> 
> Also, is it necessary to have two different methods?
> You could instead have
> 
> type Converter interface {
>     Convert(index int, v interface{}) (interface{}, os.Error)
> }
> 
> Maybe leave this out for round 1.

Cleaned this up.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:93: ColumnCoverter(idx int) ValueConverter
On 2011/09/16 15:17:58, rsc wrote:
> Converter is spelled wrong

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:125: var DDLSuccess Result = ddlSuccess{}
On 2011/09/16 15:17:58, rsc wrote:
> ?

documented.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go
File src/pkg/exp/sql/sql.go (right):

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:156: func (db *DB) Prepare(query string) (*Stmt,
os.Error) {
On 2011/09/16 15:17:58, rsc wrote:
> Comments on public API functions?

Done x 15 or so.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:235: func (db *DB) Begin() (*Tx, os.Error) {
On 2011/09/16 04:16:58, crawshaw wrote:
> There are different types of transactions (e.g. deferred, immediate,
exclusive).
> How about Begin(arg string)?

We'll keep Begin() for the common case (driver-preferred default) but we can add
a BeginXxx(someType) method in the future.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:240: // This is non-portable and should only be used when
On 2011/09/16 15:17:58, rsc wrote:
> Delete this sentence?  It's not clear what non-portable means
> and "when needed" is of course a matter of judgment.

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:255: func (tx *Tx) Rollback() os.Error {
On 2011/09/16 04:16:58, crawshaw wrote:
> When a DB supports nested transactions, you can do partial rollbacks to named
> savepoints:
> 
> http://www.sqlite.org/lang_savepoint.html
> 
> (This is common with PostgreSQL, Oracle, MSSQL, etc.)

Let's do this also in round 2.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:297: // Exec executes a prepared statement with the
provided placeholder arguments
On 2011/09/16 15:17:58, rsc wrote:
> // Exec executes a prepared statement with the given arguments and
> // returns a Result summarizing the effect of the statement.

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:375: // Query executes a prepared statement with the
provided placeholder arguments
On 2011/09/16 15:17:58, rsc wrote:
> // Query executes a prepared query statement with the given arguments
> // and returns the query results as a *Rows.

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:399: // QueryRow wraps Query, for use in selecting a
single row. Any errors
On 2011/09/16 15:17:58, rsc wrote:
> // QueryRow executes a prepared query statement with the given arguments.
> // If an error occurs during the execution of the statement, that error will
> // be returned by a call to Scan on the returned *Row, which is always
non-nil.
> // If the query selects no rows, the *Row's Scan will return ErrNoRows.
> // Otherwise, the *Row's Scan scans the first selected row and discards
> // the rest.

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:459: // Next advances the Rows' cursor (which starts
before the first
On 2011/09/16 15:17:58, rsc wrote:
> ' without s is only for plurals.  Rows is a singular here.

My memory and the Internet seem to imply it's either way with singular
possessive ending in es.  But the preferred way seems to be /'s$/.

> but we can avoid the term cursor entirely, to avoid
> people thinking this is a JDBC cursor.

SGTM


> 
> // Next prepares the next result row for reading with the Scan method. 
> // It returns true on success, false if there is no next result row.
> // Every call to Scan, even the first one, must be preceded by a call to Next.
> 

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:484: // Scan copies the columns in the current row into
variables referened in dest.
On 2011/09/16 15:17:58, rsc wrote:
> into the values pointed at by dest.

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:520: // Row is the result of calling QueryRow to select a
single row. If no
On 2011/09/16 15:17:58, rsc wrote:
> I think you can drop the second sentence, leaving it for QueryRow

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:529: // Scan copies the columns from the matched row into
the variables
On 2011/09/16 15:17:58, rsc wrote:
> // Scan copies the columns from the matched row into the values
> // pointed at by dest.  If more than one row matches the query,
> // Scan uses the first row and discards the rest.  If no row matches
> // the query, Scan returns ErrNoRows.

Done.

http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:542: type Result interface {
On 2011/09/16 15:17:58, rsc wrote:
> // A Result summarizes an executed SQL command.

Done.

[bradfitz, 2011-09-29 01:09:09]

Hello gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net,
kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com,
fw@deneb.enyo.de, r@golang.org, r@google.com, david.crawshaw@zentus.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-09-29 01:28:37]

LGTM within my limited understanding.
there's some nice stuff in here.
the interfaces are somewhat inconsistently described. the type should be
presented quickly, then each method described thoroughly.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go
File src/pkg/exp/sql/convert.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n...
src/pkg/exp/sql/convert.go:28: // copyConvert copies to dest the value in src,
converting it if possible
s/$/./

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n...
src/pkg/exp/sql/convert.go:46: *d = s
this isn't a copy.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt
File src/pkg/exp/sql/doc.txt (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc...
src/pkg/exp/sql/doc.txt:23: * To type casting/conversions consistently between
all drivers. To
i can't parse the first sentence. (maybe just s/To t/T/ so it's not an
infinitive, in parallel with the other entries.)
maybe
Make type casting and conversion consistent between all drivers.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc...
src/pkg/exp/sql/doc.txt:24: achieve this, most of the type conversions are done
in the db
s/type //

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
File src/pkg/exp/sql/driver/driver.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:38: // Driver is the interface that must be
implemented by database
s/by/by a/

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:55: // drivers which can provide a more
effcient implementation.
s/which/that/
s/effcient/efficient/

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:86: LastInsertId() (int64, os.Error)
document these. the off-by-oneness should be documented too

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:101: // type as defined above.
second sentence doesnt' parse.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:104: // Exec executes a query that may rows,
such as a SELECT.  The
"that may rows"?  "may return rows"?

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:122: // Columns returns the name of the
columns. The number of
s/name/names/

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:125: // string should be returned.
s/.$/ for that entry./

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types.go
File src/pkg/exp/sql/driver/types.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types...
src/pkg/exp/sql/driver/types.go:76: return nil, fmt.Errorf("unsupported value %v
(type %T) converting to int32", v, v)
here and throughout the errors could start with "sql: " (or other name as
appropriate)

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types...
src/pkg/exp/sql/driver/types.go:117: // IsScanSubsetType returns if v is of a
valid type for a
s/returns if/reports whether/

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go
File src/pkg/exp/sql/sql.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:34: // a duplicate name.
instead of the second sentence:
// If Register is called twice with the same name or if driver is nil, it
panics.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:45: // NullableString is type representing a string which
may be
s/type/a type/
or jsut
NullableString represents

s/which/that/

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:46: // null. NullableString implements the ScannerInto
inteface so can be
s/can/it can/

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:74: // ScannerInto is an interface used by Scan.
this seems to be the crux, so i suggest a better doc here and a proper
description of ScanInto within this type declaration

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:181: // Exec executes a query without returning any rows.
i don't know what this means, but i'm not a db expert. does it mean that the
query cannot return rows, or that we don't care if it does?

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:298: type Stmt struct {
some of these names are short, like Exec and Stmt. that jangles a bit, but it's
probably ok. is this nomenclature what people expect?

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:308: func todo() string {
lovely

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:529: // if encountered normally the Rows are closed
automatically. Calling
s/if/is/ ?
s/normally/&, /
s/Calling//

[bradfitz, 2011-09-29 18:43:25]

All done.

Russ, thoughts on this as a checkpoint?

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go
File src/pkg/exp/sql/convert.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n...
src/pkg/exp/sql/convert.go:28: // copyConvert copies to dest the value in src,
converting it if possible
On 2011/09/29 01:28:37, r wrote:
> s/$/./

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n...
src/pkg/exp/sql/convert.go:46: *d = s
On 2011/09/29 01:28:37, r wrote:
> this isn't a copy.

It shouldn't.  I've renamed the function to "convertAssign" This part (scanning
lots of rows quickly from a table) should be able to work purely in []byte
end-to-end, without copies.  Added some docs, too.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt
File src/pkg/exp/sql/doc.txt (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc...
src/pkg/exp/sql/doc.txt:1: Goals of the db and db/dbimpl packages:
updated all the db & dbimpl to sql & driver, too

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc...
src/pkg/exp/sql/doc.txt:23: * To type casting/conversions consistently between
all drivers. To
On 2011/09/29 01:28:37, r wrote:
> i can't parse the first sentence. (maybe just s/To t/T/ so it's not an
> infinitive, in parallel with the other entries.)
> maybe
> Make type casting and conversion consistent between all drivers.

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc...
src/pkg/exp/sql/doc.txt:23: * To type casting/conversions consistently between
all drivers. To
On 2011/09/29 01:28:37, r wrote:
> i can't parse the first sentence. (maybe just s/To t/T/ so it's not an
> infinitive, in parallel with the other entries.)
> maybe
> Make type casting and conversion consistent between all drivers.

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc...
src/pkg/exp/sql/doc.txt:24: achieve this, most of the type conversions are done
in the db
On 2011/09/29 01:28:37, r wrote:
> s/type //

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
File src/pkg/exp/sql/driver/driver.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:38: // Driver is the interface that must be
implemented by database
On 2011/09/29 01:28:37, r wrote:
> s/by/by a/

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:55: // drivers which can provide a more
effcient implementation.
On 2011/09/29 01:28:37, r wrote:
> s/which/that/
> s/effcient/efficient/

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:86: LastInsertId() (int64, os.Error)
On 2011/09/29 01:28:37, r wrote:
> document these. the off-by-oneness should be documented too

Done.  But... what off-by-oneness?

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:101: // type as defined above.
On 2011/09/29 01:28:37, r wrote:
> second sentence doesnt' parse.

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:104: // Exec executes a query that may rows,
such as a SELECT.  The
On 2011/09/29 01:28:37, r wrote:
> "that may rows"?  "may return rows"?

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:122: // Columns returns the name of the
columns. The number of
On 2011/09/29 01:28:37, r wrote:
> s/name/names/

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:125: // string should be returned.
On 2011/09/29 01:28:37, r wrote:
> s/.$/ for that entry./

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types.go
File src/pkg/exp/sql/driver/types.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types...
src/pkg/exp/sql/driver/types.go:76: return nil, fmt.Errorf("unsupported value %v
(type %T) converting to int32", v, v)
On 2011/09/29 01:28:37, r wrote:
> here and throughout the errors could start with "sql: " (or other name as
> appropriate)

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types...
src/pkg/exp/sql/driver/types.go:117: // IsScanSubsetType returns if v is of a
valid type for a
On 2011/09/29 01:28:37, r wrote:
> s/returns if/reports whether/

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go
File src/pkg/exp/sql/sql.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:34: // a duplicate name.
On 2011/09/29 01:28:37, r wrote:
> instead of the second sentence:
> // If Register is called twice with the same name or if driver is nil, it
> panics.

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:45: // NullableString is type representing a string which
may be
On 2011/09/29 01:28:37, r wrote:
> s/type/a type/
> or jsut
> NullableString represents
> 
> s/which/that/

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:46: // null. NullableString implements the ScannerInto
inteface so can be
On 2011/09/29 01:28:37, r wrote:
> s/can/it can/

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:74: // ScannerInto is an interface used by Scan.
On 2011/09/29 01:28:37, r wrote:
> this seems to be the crux, so i suggest a better doc here and a proper
> description of ScanInto within this type declaration

Done.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:181: // Exec executes a query without returning any rows.
On 2011/09/29 01:28:37, r wrote:
> i don't know what this means, but i'm not a db expert. does it mean that the
> query cannot return rows, or that we don't care if it does?

Expanded the comment.  INSERT, UPDATE etc.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:298: type Stmt struct {
On 2011/09/29 01:28:37, r wrote:
> some of these names are short, like Exec and Stmt. that jangles a bit, but
it's
> probably ok. is this nomenclature what people expect?

I don't know what people expect, nor have a preference.  I think rsc proposed
these names originally, but maybe whiteboard brevity was the cause.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:308: func todo() string {
On 2011/09/29 01:28:37, r wrote:
> lovely

this was critical when I started with dozens of TODOs.  it got hard to figure
out.

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco...
src/pkg/exp/sql/sql.go:529: // if encountered normally the Rows are closed
automatically. Calling
On 2011/09/29 01:28:37, r wrote:
> s/if/is/ ?
> s/normally/&, /
> s/Calling//

Done.

[bradfitz, 2011-09-29 18:43:38]

Hello gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net,
kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com,
fw@deneb.enyo.de, r@golang.org, r@google.com, david.crawshaw@zentus.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-09-29 23:01:46]

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
File src/pkg/exp/sql/driver/driver.go (right):

http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive...
src/pkg/exp/sql/driver/driver.go:86: LastInsertId() (int64, os.Error)
insertion goes into the gaps. is LastInsertId the one we inserted, the one
after, the one before, the one we'll do next, ....? is RowsAffected a count?

[rsc, 2011-09-29 23:02:15]

LGTM

Fix copyright notices.

[bradfitz, 2011-09-29 23:12:31]

*** Submitted as http://code.google.com/p/go/source/detail?r=d21caa7909f2 ***

exp/sql{,/driver}: new database packages

R=gustavo, rsc, borman, dave, kevlar, nigeltao, dvyukov, kardianos, fw, r, r,
david.crawshaw
CC=golang-dev
http://codereview.appspot.com/4973055