code review 5294067: exp/sql: finish transactions, flesh out types, docs

exp/sql: finish transactions, flesh out types, docs

Fixes issue 2328 (float, bool)

[bradfitz, 2011-10-25 23:53:06]

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

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

[bradfitz, 2011-10-31 17:48:52]

ping?

On Tue, Oct 25, 2011 at 4:53 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com, rsc,
>
> Message:
> Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> exp/sql: finish transactions, flesh out types, docs
>
> Fixes issue 2328 (float, bool)
>
> Please review this at
http://codereview.appspot.com/**5294067/<http://codereview.appspot.com/5294067/>
>
> Affected files:
>  M src/pkg/Makefile
>  M src/pkg/exp/sql/convert.go
>  M src/pkg/exp/sql/convert_test.**go
>  M src/pkg/exp/sql/driver/driver.**go
>  M src/pkg/exp/sql/driver/types.**go
>  A src/pkg/exp/sql/driver/types_**test.go
>  M src/pkg/exp/sql/fakedb_test.go
>  M src/pkg/exp/sql/sql.go
>
>
>

[bradfitz, 2011-11-02 18:24:12]

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

Please take another look.

[rsc, 2011-11-02 18:30:31]

LGTM

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

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/sql.go#newcod...
src/pkg/exp/sql/sql.go:238: // Begin starts a transaction. The isolation level
is dependent on
What does the second sentence mean?

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/sql.go#newcod...
src/pkg/exp/sql/sql.go:332: // TODO(bradfitz): the restriction that the returned
statement
It sounds like the methods on Stmt - Exec, Query, QueryRow - should
really be methods on DB and Tx, maybe named ExecStmt, QueryStmt, QueryRowStmt,
that take the *Stmt as the first argument.

Leave for a different CL but I agree something has to change.

[r, 2011-11-02 18:33:41]

LGTM

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

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/driver/types....
src/pkg/exp/sql/driver/types.go:21: //    fits. (for instance, making sure a
particular int64 fits in a
s/. (/, /
maybe s/for instance,/such as/

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/driver/types....
src/pkg/exp/sql/driver/types.go:22: //    table's uint16 column)
s/)/./

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

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/sql.go#newcod...
src/pkg/exp/sql/sql.go:433: mu     sync.Mutex // protects following
following one, or all the rest?

[bradfitz, 2011-11-02 18:33:48]

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

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/sql.go#newcod...
src/pkg/exp/sql/sql.go:238: // Begin starts a transaction. The isolation level
is dependent on
On 2011/11/02 18:30:31, rsc wrote:
> What does the second sentence mean?

http://en.wikipedia.org/wiki/Isolation_(database_systems)#Isolation_levels

It's a TODO to add support for Begin at a specified level.

[bradfitz, 2011-11-02 18:40:04]

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

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/driver/types....
src/pkg/exp/sql/driver/types.go:21: //    fits. (for instance, making sure a
particular int64 fits in a
On 2011/11/02 18:33:41, r wrote:
> s/. (/, /
> maybe s/for instance,/such as/

Done.

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/driver/types....
src/pkg/exp/sql/driver/types.go:22: //    table's uint16 column)
On 2011/11/02 18:33:41, r wrote:
> s/)/./

Done.

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

http://codereview.appspot.com/5294067/diff/7001/src/pkg/exp/sql/sql.go#newcod...
src/pkg/exp/sql/sql.go:433: mu     sync.Mutex // protects following
On 2011/11/02 18:33:41, r wrote:
> following one, or all the rest?

rest. done.

[bradfitz, 2011-11-02 18:46:08]

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

exp/sql: finish transactions, flesh out types, docs

Fixes issue 2328 (float, bool)

R=rsc, r
CC=golang-dev
http://codereview.appspot.com/5294067

[rsc, 2011-11-02 18:58:57]

On Wed, Nov 2, 2011 at 14:33,  <bradfitz@golang.org> wrote:
> It's a TODO to add support for Begin at a specified level.

what are the levels?
i thought isolated was isolated.

[bradfitz, 2011-11-02 19:12:13]

On Wed, Nov 2, 2011 at 11:58 AM, Russ Cox <rsc@golang.org> wrote:

> On Wed, Nov 2, 2011 at 14:33,  <bradfitz@golang.org> wrote:
> > It's a TODO to add support for Begin at a specified level.
>
> what are the levels?
> i thought isolated was isolated.
>

I linked them above.  Probably got URL noise from rietveld, so:

           http://en.wikipedia.org/wiki/**Isolation_(database_systems)#**
Isolation_levels<http://en.wikipedia.org/wiki/Isolation_(database_systems)#Isolation_levels>

[rsc, 2011-11-02 19:23:01]

oh, right.  the 4 levels are
   isolated
   not isolated
   not isolated
   not isolated

:-)

russ