database/sql: deadlock with simultaneous queries #3857
Labels
Milestone
Comments
|
Labels changed: added priority-soon, removed priority-triage. Owner changed to @bradfitz. Status changed to Accepted. |
I'm not sure whether this should even work. Will all databases support this? The current behavior is basically as designed: all operation inside a transaction use a connection exclusively. The database/sql/driver package promises that: "Conn is a connection to a database. It is not used concurrently by multiple goroutines." This might just be a documentation fix. Alex, did you have an alternate proposal? Labels changed: removed priority-soon. Status changed to Thinking. |
I think my example is trivial. I can't see how you can convince someone that it "should not work". Whole point of having database transaction is to create temporary but fixed environment where multiple database statements can be executed, illusion of "stable" world. If you think about changing the doco, what would your new doco say? If you ask for my opinion, I think you are doing too much inside of database/sql. In my opinion, drivers is where all these decisions should be made. I suspect, most drivers would handle my example with no problem, while you are proposing to "disallow" it. Alex |
Sorry, but the design of the database/sql package is to provide a least common denominator of the 80% of operations that are most used & needed while making it easy for people to implement drivers. You're proposing ditching that promise and pushing complexity down to the drivers. Your example code works if you close the first query before doing the second query in the transaction. Does SQLite even support concurrent queries within a transaction? It doesn't even support concurrent queries at all, really. I'm fine making this work concurrently if a driver opts-in and says they support it, but it can't work with the current sql/driver promise. I'm sorry you don't like the sql package. You can use databases directly if you don't like the existing abstractions. It's a trade-off between flexibility & pain vs simplicity & easy. |
I think your loyalty is misplaced :-), it should be with database/sql users, not with driver writers. I hope we should have many of former and just a couple of later. Regardless of how silly my example looks, the fact that it compiles and runs and then crashes with "throw: all goroutines are asleep - deadlock!" is not a good look on our part. I do not know enough about SQLite to answer your question, but I will investigate when I have time. Either way, if SQLite does not support this, the driver can just say so by failing. Driver opts-in sounds like a great plan to me. Sure, I can write my own database access package, when I have unlimited time :-). You created database/sql package while you had no "real" drivers for it. It is possible that you have made some mistakes. Alex |
Brad, simple sqlite example attached. As much as I know about sqlite, but it looks OK to me. Alex Attachments:
|
@rsc, " ... should return an error in the second Query call, because there is already an active query ...". I think it is way too restrictive. Whole "transaction" idea is about executing "multiple" actions inside one single context. Are you saying these actions have to run serially? Why? If database supports parallel execution, why would we restrict that interface. If you decide to proceed with this idea, then our api should change to block in these situations. Current api allows creation of 2 rowsets, they should both work. Why would you allow one to work, but the other fail? Alex |
If you run two simultaneous queries outside a transaction they basically run in different implicit transactions. It is far from clear to me that you are allowed to try to do multiple parallel things inside a transaction rather than doing them in sequence. Transactions are about grouping things, not necessarily running them in parallel. |
> ... If you run two simultaneous queries outside a transaction they basically run in different implicit transactions. True. But how is it related to our discussion? > ... It is far from clear to me that you are allowed to try to do multiple parallel things inside a transaction rather than doing them in sequence. Transactions are about grouping things, not necessarily running them in parallel. ... I agree that "ransactions are about grouping things". I think that nothing else should matter. It is up to database to decide if parallel or sequential statement execution is in order. It should not be enforced by our database/sql package. Alex |
It is an explanation of why parallelism works outside transactions: because it is treated as multiple transactions. We should allow what the database engines can handle. Within a single transaction I believe that it is plausible for a database to require sequential use of queries. If some databases deadlock when you try to run a second query in a transaction without consuming the results of the first, then database/sql should detect this case and give a nice error instead of letting things deadlock. That's all I'm saying. I'm not trying to artificially limit what the package can do, just to give a better error if indeed the database cannot handle what is being asked. Russ |
> ... We should allow what the database engines can handle. Great. > ... Within a single transaction I believe that it is plausible for a database to require sequential use of queries. ... I disagree. I have implemented ODBC go driver recently http://code.google.com/p/odbc/. There are quite a few ODBC industrial database drivers (MS SQL Server, IBM DB2, Oracle and many others). Nowhere in ODBC documentation did I see any restriction on "concurrent or parallel" API execution - everything is expressed in connections, statements and rowsets. You get token for each, you do what is acceptable to do, you release the token back to the driver manager. I have also tried to write program that does "simultaneous" queries against sqlite (https://golang.org/issue/3857?c=7) and I see no complains. > ... If some databases deadlock when you try to run a second query in a transaction without consuming the results of the first, then database/sql should detect this case and give a nice error instead of letting things deadlock. ... I am certain, most databases will detect deadlock and will provide appropriate message. > ... I'm not trying to artificially limit what the package can do, ... But you are. See my TestSimultaneousQueries fails with "throw: all goroutines are asleep - deadlock!". The message should be originated by code implementing database/sql/driver, not database/sql itself. > ... just to give a better error if indeed the database cannot handle what is being asked. I say, leave it to the database to deal with that. I would say more, I can imagine a situation where transaction would deadlock even when all statements are executed sequentially: 1) tran1 updates record1 in table1; 2) tran2 updates record2 in table2; 3) tran1 updates record2 in table2; 4) tran2 updates record1 in table1; This will deadlock, but database is smart enough and will fail one of tran1 or tran2 transactions and let the other proceed. We can't diagnose these. We shouldn't even try to. Alex |
I'm new to Go, but very familiar with relational databases. The thinking on this topic is very, very wrong from everyone but Alex. Alex is absolutely right. Every database driver supports multiple actions being performed in a single transaction on a single connection at the same time. If you want a real-world example: I have a small app that is supposed to migrate+massage+merge data from one table (that's loaded annually directly from a SQL file) to a master table. Within the transaction I have one query that runs against my source table, cycling through each record, and other queries check to see if the record already exists (the data for the source table is updated annually). I need to run this all in a transaction, so that if any update fails it all fails. Unfortunately, Go deadlocks on this second query, which means this scenario/app is not feasible in Go unless I risk doing things outside of a transaction. For what it's worth, as an exercise I've rewritten the app in Python, Ruby, Java, and JavaScript (Node). Works in every other language/driver with nearly the exact same logic. I thought I was going crazy. Turns out it's just Go that's crazy. |
This issue is still open. Please refrain from non-constructive comments. If you have something useful to say, you're welcome to participate in the project. Feel free to share your Python, Ruby, Java, and JavaScript implementations, for instance. The current thinking on this bug is that we won't grab a mutex and deadlock like now, but the database/sql will keep its promise to drivers and not used a driver.Conn concurrently unless the driver opts-in to it. Regardless of the driver's choice, the *sql.Tx may also be used concurrently, but the sql package may serialize queries for drivers onto the same Conn if no alternative is provided by the driver. (The driver could choose to allow concurrent use of the same Conn, or even spread a transaction out over multiple Conns) |
Sorry, I was trying to give a more real-world example so you could see how this issue is problematic. Here's my Java and my JavaScript versions. The Ruby version has too many files and requires too much fluff (ActiveRecord, ActiveSupport, etc.), as does the Python version (SQLAlchemy, etc.), so I left those out. The Java version is straight JDBC with the PostgreSQL driver. The JavaScript version uses any-db and pg. I also attached the Go version. It doesn't work (hangs almost immediately), but maybe it can serve as a reference. Like I said, I'm new to Go, so apologies if my coding style isn't up to snuff there. Attachments:
|
Sent out https://golang.org/cl/7803043/ as a pre-req fo rthis. |
|
This issue was updated by revision f28c8fb. R=golang-dev, adg CC=golang-dev https://golang.org/cl/7803043 |
CL created: https://golang.org/cl/7707047 |
|
This issue was closed by revision a7a803c. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
What steps will reproduce the problem? 1. apply this diff to the go tree (hg id is ddaabb722563+) diff -r ddaabb722563 src/pkg/database/sql/sql_test.go --- a/src/pkg/database/sql/sql_test.go Sun Jul 22 16:35:53 2012 -0700 +++ b/src/pkg/database/sql/sql_test.go Tue Jul 24 17:33:24 2012 +1000 @@ -415,6 +415,29 @@ } } +func TestSimultaneousQueries(t *testing.T) { + db := newTestDB(t, "people") + defer closeDB(t, db) + + tx, err := db.Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + + r1, err := tx.Query("SELECT|people|name|") + if err != nil { + t.Fatal(err) + } + defer r1.Close() + + r2, err := tx.Query("SELECT|people|name|") + if err != nil { + t.Fatal(err) + } + defer r2.Close() +} + // Tests fix for issue #2788, that we bind nil to a []byte if the // value in the column is sql null func TestNullByteSlice(t *testing.T) { 2. run "go test -v -run=Sim" command What is the expected output? Test should PASS What do you see instead? throw: all goroutines are asleep - deadlock! goroutine 1 [chan receive]: testing.RunTests(0x8048c00, 0x81f2c00, 0x17, 0x17, 0xb76cbe01, ...) /root/go/root/src/pkg/testing/testing.go:356 +0x72e testing.Main(0x8048c00, 0x81f2c00, 0x17, 0x17, 0x81f61c0, ...) /root/go/root/src/pkg/testing/testing.go:291 +0x5d main.main() /tmp/go-build698477669/database/sql/_test/_testmain.go:87 +0x51 goroutine 2 [syscall]: created by runtime.main /root/go/root/src/pkg/runtime/proc.c:220 goroutine 3 [semacquire]: sync.runtime_Semacquire(0x18725cb8, 0x1) /root/go/root/src/pkg/runtime/zsema_linux_386.c:146 +0x29 sync.(*Mutex).Lock(0x18725cb4, 0x806eb09) /root/go/root/src/pkg/sync/mutex.go:60 +0xb8 database/sql.(*Tx).grabConn(0x18725ca0, 0x18700a40, 0x18701960) /root/go/root/src/pkg/database/sql/sql.go:478 +0x53 database/sql.(*Tx).Prepare(0x18725ca0, 0x8136ea8, 0x13, 0x0, 0x0, ...) /root/go/root/src/pkg/database/sql/sql.go:524 +0x39 database/sql.(*Tx).Query(0x18725ca0, 0x8136ea8, 0x13, 0x0, 0x0, ...) /root/go/root/src/pkg/database/sql/sql.go:623 +0x5a database/sql.TestSimultaneousQueries(0x18700900, 0xe) /root/go/root/src/pkg/database/sql/sql_test.go:434 +0x1f2 testing.tRunner(0x18700900, 0x81f2cb4, 0x0) /root/go/root/src/pkg/testing/testing.go:279 +0x71 created by testing.RunTests /root/go/root/src/pkg/testing/testing.go:355 +0x711The text was updated successfully, but these errors were encountered: