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

Comments

Projects
None yet
4 participants
@rittneje
Copy link

rittneje commented Dec 31, 2018

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

This comment has been minimized.

Copy link
Member

agnivade commented Dec 31, 2018

@rittneje

This comment has been minimized.

Copy link
Author

rittneje commented Dec 31, 2018

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

This comment has been minimized.

Copy link
Contributor

kardianos commented Dec 31, 2018

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

This comment has been minimized.

Copy link
Author

rittneje commented Dec 31, 2018

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

This comment has been minimized.

Copy link
Contributor

kardianos commented Dec 31, 2018

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

This comment has been minimized.

Copy link
Author

rittneje commented Dec 31, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.