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
Comments
/cc @kardianos |
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()
... |
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.
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. |
I think adding some additional information somewhere is warranted. (My preference would be on db.SetMaxIdleConns(1)
db.SetMaxOpenConns(1)
db.SetConnMaxLifetime(0) I expected 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. |
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. |
Yes, this is the behavior I expected, as it matches what happens when you "disable" the connection pool. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
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 aQuery
/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.
The text was updated successfully, but these errors were encountered: