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: deadlock with simultaneous queries #3857

Closed
alexbrainman opened this issue Jul 24, 2012 · 25 comments
Closed

database/sql: deadlock with simultaneous queries #3857

alexbrainman opened this issue Jul 24, 2012 · 25 comments
Assignees
Milestone

Comments

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jul 24, 2012

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 +0x711
@robpike
Copy link
Contributor

@robpike robpike commented Jul 24, 2012

Comment 1:

Labels changed: added priority-soon, removed priority-triage.

Owner changed to @bradfitz.

Status changed to Accepted.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 7, 2012

Comment 2:

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.

@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Aug 7, 2012

Comment 3:

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
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 7, 2012

Comment 4:

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.
@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Aug 7, 2012

Comment 5:

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
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 7, 2012

Comment 6:

Okay.
@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Aug 8, 2012

Comment 7:

Brad, simple sqlite example attached. As much as I know about sqlite, but it looks OK to
me.
Alex

Attachments:

  1. sqlite_test.go (1599 bytes)
@rsc
Copy link
Contributor

@rsc rsc commented Sep 12, 2012

Comment 8:

I think Alex's original example should return an error in the second Query call, because
there is already an active query.
@rsc
Copy link
Contributor

@rsc rsc commented Sep 12, 2012

Comment 9:

Labels changed: added go1.1.

@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Sep 13, 2012

Comment 10:

@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
@rsc
Copy link
Contributor

@rsc rsc commented Sep 13, 2012

Comment 11:

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.
@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Sep 13, 2012

Comment 12:

> ... 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
@rsc
Copy link
Contributor

@rsc rsc commented Sep 13, 2012

Comment 13:

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
@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Sep 14, 2012

Comment 14:

> ... 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
@rsc
Copy link
Contributor

@rsc rsc commented Sep 14, 2012

Comment 15:

Okay, great. If it's just a dumb bug in the wrapper that's fine. I
thought it was something more fundamental, that the wrapper had
deadlocked because the underlying database had deadlocked.
@rsc
Copy link
Contributor

@rsc rsc commented Dec 10, 2012

Comment 16:

Labels changed: added size-m.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 27, 2013

Comment 17 by pico303:

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.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 27, 2013

Comment 18:

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)
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 27, 2013

Comment 19 by pico303:

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:

  1. MigrateNoaa.java (1688 bytes)
  2. migrate_noaa.js (1397 bytes)
  3. noaa.go (2861 bytes)
@rsc
Copy link
Contributor

@rsc rsc commented Mar 12, 2013

Comment 20:

Labels changed: added go1.1maybe, removed go1.1.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 13, 2013

Comment 21:

Status changed to Started.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 13, 2013

Comment 22:

Sent out https://golang.org/cl/7803043/ as a pre-req fo rthis.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 14, 2013

Comment 23:

This issue was updated by revision f28c8fb.

R=golang-dev, adg
CC=golang-dev
https://golang.org/cl/7803043
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 18, 2013

Comment 24:

CL created: https://golang.org/cl/7707047
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 18, 2013

Comment 25:

This issue was closed by revision a7a803c.

Status changed to Fixed.

@rsc rsc added this to the Go1.1 milestone Apr 14, 2015
@rsc rsc removed the go1.1maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.