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

database/sql: DB.Conn violates isolation #29483

Closed
rittneje opened this issue Dec 31, 2018 · 6 comments
Closed

database/sql: DB.Conn violates isolation #29483

rittneje opened this issue Dec 31, 2018 · 6 comments

Comments

@rittneje
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jrittner/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jrittner/go-sqlite3"
GORACE=""
GOROOT="/home/jrittner/go"
GOTMPDIR=""
GOTOOLDIR="/home/jrittner/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build359303402=/tmp/go-build -gno-record-gcc-switches"

What did you do?

	db, err := sql.Open("sqlite3", "file:foo.sqlite?cache=private&mode=rwc")
	if err != nil {
		panic(err)
	}
	defer db.Close()

	_, err = db.Exec("DROP TABLE IF EXISTS src")
	if err != nil {
		panic(err)
	}

	_, err = db.Exec("CREATE TABLE src (id INTEGER NOT NULL PRIMARY KEY, data1 INTEGER, data2 INTEGER);")
	if err != nil {
		panic(err)
	}

	ctx := context.Background()

	c, err := db.Conn(ctx)
	if err != nil {
		panic(err)
	}
	defer c.Close()

	tx, err := c.BeginTx(ctx, nil)
	if err != nil {
		panic(err)
	}
	defer tx.Rollback()

	_, err = tx.Exec("INSERT INTO src (id, data1, data2) VALUES (1, 100, 1000)")
	if err != nil {
		panic(err)
	}

	r := c.QueryRowContext(ctx, "SELECT COUNT(*) FROM src LIMIT 1")
	var count int
	if err := r.Scan(&count); err != nil {
		panic(err)
	}

	fmt.Println(count)

What did you expect to see?

I expected to either see an error from QueryRowContext/Scan that the conn was busy, or a result of 0.

What did you see instead?

I see a result of 1.

This is because Conn allows non-transaction methods to be called while it is in the middle of a transaction. Also, I see no way for a driver to tell the difference between a Query/Exec that comes from the transaction vs. one that doesn't, so I don't consider this to be a driver issue.

This may be considered "user error" rather than a true bug (since I did force it to use the same conn for both), in which case it might be helpful to mention this in the documentation.

@agnivade
Copy link
Contributor

/cc @kardianos

@rittneje
Copy link
Author

Also of note: it allows you to start multiple transactions on the same conn. Previously this could never happen, so in theory there could be drivers that don't handle this properly.

db, err := sql.Open("sqlite3", "file:foo.sqlite?cache=private&mode=rwc")
if err != nil {
    panic(err)
}

c, err := db.Conn(ctx)
if err != nil {
    panic(err)
}
defer c.Close()

tx1, err := c.BeginTx(ctx, nil)
if err != nil {
    panic(err)
}
defer tx1.Rollback()

// This calls down into driver.Conn.Begin.
tx2, err := c.BeginTx(ctx, nil)
if err != nil {
    panic(err)
}
defer tx2.Rollback()

...

@kardianos
Copy link
Contributor

I agree you should understand how database sessions work before using a relational database with that uses sessions, which is most of them. A transaction is a state within a session.

Previously, we didn't allow taking a session by itself. However, some actions, such as processing a series of database scripts that must be processed in different batches, but on the same session, was impossible.

We could prevent this, but it would also limit the API yet again and be a breaking change. We could add a bit of documentation on relational database 101 theory so users are less surprised about it.

  • DB Sessions
    • DB Transactions
    • DB Statement Batch
  • DB Prepared Statements
  • DB Connection Pooling

I don't think I'd want to have a specific "warning" on the DB.Conn() call itself. Another thing users sometimes mix up is they consider a *sql.DB a "connection" rather then a connection pool. Another thing we could cover is that making an explicit prepared statement isn't necessary to create a paramaratized query.

I'd be okay with adding basic theory, probably not okay with adding a "warning" to DB.Conn, and not okay with changing this behavior.

@rittneje
Copy link
Author

I think adding some additional information somewhere is warranted. (My preference would be on sql.Conn itself since that's where I'd look for it.) The connection pool concept has been a constant source of issues with SQLite, so at present we effectively disable pooling by doing the following:

db.SetMaxIdleConns(1)
db.SetMaxOpenConns(1)
db.SetConnMaxLifetime(0)

I expected db.Conn to result in similar semantics, but obviously it does not.

I still maintain that allowing a second transaction to begin is particularly problematic because you could corrupt a driver that wasn't written to expect that to happen.

@kardianos
Copy link
Contributor

If you are using SQLite, you may be interested in https://crawshaw.io/blog/one-process-programming-notes and https://godoc.org/crawshaw.io/sqlite .

The docs for DB.Conn clearly state it opens a database session. I'm not sure what other behavior you would expect. I suppose an explicit conn.Begin() could block all other non-transaction queries until the rollback or commit. But that would seems kinda odd too.

@rittneje
Copy link
Author

I suppose an explicit conn.Begin() could block all other non-transaction queries until the rollback or commit.

Yes, this is the behavior I expected, as it matches what happens when you "disable" the connection pool.

@katiehockman katiehockman added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 2, 2019
@katiehockman katiehockman added this to the Go1.13 milestone Jan 2, 2019
@kardianos kardianos added Proposal-Declined and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Apr 5, 2019
@golang golang locked and limited conversation to collaborators May 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants