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: Erroneous ErrNoRows when using QueryRow("INSERT INTO ... RETURNING ...") when violating database constraint. #6651

Closed
gopherbot opened this issue Oct 23, 2013 · 21 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 23, 2013

by lee.hambley:

Steps to reproduce the problem:

If possible, include a link to a program on play.golang.org.

1. Use PostgreSQL
2. Use the lib/pq driver (latest, `78760269eebe5e70b28bb94bff676d7b28df275d
src/github.com/lib/pq (heads/master)`), 
3. Use a table with a constraint
4. Violate that constraint when inserting using RETURNING. (i.e duplicate unique key)
5. Notice that when `sql.DB.QueryRow().Scan()` is used, `ErrNoRows` is returned before
errors are correctly checked for.

What is the expected output?

It is expected that `sql.QueryRow` will return a stub `*sql.Row` which will return an
error of type `*pq.Error` representing the constraint violation error. Typically
something like this https://github.com/lib/pq/blob/master/error.go#L157

What do you see instead?

`sql.QueryRow()` does not properly handle this case. It first checks for number of
returned rows (none, as we have failed to insert), and then returns `sql.ErrNoRows`
before checking if there is an error returned by the query.

Which compiler are you using (5g, 6g, 8g, gccgo)?

6g (standard Go code, no cgo as far as I am aware, `go build -x my/package` isn't
showing anything useful)

Which operating system are you using?

OSX 10.9 Mavericks 64bit

Which version are you using?  (run 'go version')

go version go1.1.2 darwin/amd64

Please provide any additional information below.

There is an extensive Github thread (closed)
[here](lib/pq#77, discussing first, a similar issue, and then
re-opened with my findings at [this
comment](lib/pq#77 (comment). Specifically
someone has identified a possible weakness in the error checking of `func (rs *Rows)
Next() bool`, where `rs.lasterr` is only checked against `io.EOF` before being
overwritten and returned as `nil`.

The attached file isn't a complete running piece of software, but it is an extracted
sample, the same that is found in the GitHub thread.

Attachments:

  1. known_issues_test.go (1191 bytes)
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Oct 24, 2013

Comment 2:

FYI. I don't see any problems with MSSQL. I added this test
diff --git a/mssql_test.go b/mssql_test.go
--- a/mssql_test.go
+++ b/mssql_test.go
@@ -1180,3 +1180,28 @@
        t.Errorf("expected \"hello\", but received %v", s)
    }
 }
+
+func Test_RaisingPqErrorOnQueryRowConstraintViolation(t *testing.T) {
+   db, sc, err := mssqlConnect()
+   if err != nil {
+       t.Fatal(err)
+   }
+   defer closeDB(t, db, sc, sc)
+
+   db.Exec("drop table dbo.temp")
+   _, err = db.Exec("CREATE TABLE dbo.temp (a varchar(3) not null UNIQUE)")
+   if err != nil {
+       t.Fatal(err)
+   }
+   var returned string
+   err = db.QueryRow("INSERT INTO dbo.temp (a) output inserted.a values(?)",
"abc").Scan(&returned)
+   if err != nil {
+       t.Fatal(err)
+   }
+   err = db.QueryRow("INSERT INTO dbo.temp (a) output inserted.a values(?)",
"abc").Scan(&returned)
+   if err == sql.ErrNoRows {
+       t.Fatal(err)
+   }
+   t.Logf("err=%v", err)
+   exec(t, db, "drop table dbo.temp")
+}
to code.google.com/p/odbc, and I get this
=== RUN Test_RaisingPqErrorOnQueryRowConstraintViolation
--- PASS: Test_RaisingPqErrorOnQueryRowConstraintViolation (0.02 seconds)
    mssql_test.go:1205: err=SQLExecute: {23000} [FreeTDS][SQL Server]Violation of UNIQUE KEY constraint 'UQ__temp__39251148'. Cannot insert duplicate key in object 'dbo.temp'.
PASS
ok      code.google.com/p/odbc  0.027s
which looks reasonable to me.
Alex
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 24, 2013

Comment 3 by marko@trustly.com:

This problem only occurs if the driver's Query() returns a nil error, but the first call
to Next() returns an actual error from the database (i.e. not io.EOF).
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 24, 2013

Comment 4 by marko@trustly.com:

Just a small correction to the original post: rs.lasterr is not overwritten, it's simply
ignored.  Attached is a fake driver and a sample program demonstrating the problem.  If
I change sql.go like this:
@@ -1422,6 +1422,9 @@
        defer r.rows.Close()
        if !r.rows.Next() {
+               if r.rows.Err() != nil {
+                       return r.rows.Err()
+               }
                return ErrNoRows
        }
        err := r.rows.Scan(dest...)
I get the expected result.
Is it too late to fix this for 1.2?

Attachments:

  1. fakedriver.go (758 bytes)
  2. ignored.go (225 bytes)
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Oct 25, 2013

Comment 5:

> This problem only occurs if the driver's Query() returns a nil error ...
And why doesn't it? If query fails, it should return error. No?
Alex
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 25, 2013

Comment 6 by marko@trustly.com:

It's not possible to know whether a query will fail or not without running it to
completion.  If Query() has to return an error in all such cases, it's not possible to
lazily fetch results from the database.
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Oct 26, 2013

Comment 7:

> It's not possible to know whether a query will fail or not without running it to
completion. ...
Fair enough. So you have to wait till it is done.
> ... If Query() has to return an error in all such cases, it's not possible to lazily
fetch results from the database. ...
Please try again. I don't understanding the meaning of this statement. Wouldn't you need
to know if Query() succeeded before attempting to fetch result records?
Alex
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 26, 2013

Comment 8 by marko@trustly.com:

> Please try again. I don't understanding the meaning of this statement. Wouldn't you
need to know if Query() succeeded before attempting to fetch result records?
No, that's my point exactly.  Consider the following query:
    SELECT (SELECT 1 FROM bar WHERE bar.f1 = foo.f1) FROM foo;
It's an error for that subquery to return more than one row, but it's impossible to know
whether it does that or not without actually fetching results from the query.  Unless it
actually starts fetching the results, Query() can't possibly know whether the query
succeeded or not.
Now: why is it okay for database/sql to ignore the error from Next()?
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 26, 2013

Comment 9 by marko@trustly.com:

>> Please try again. I don't understanding the meaning of this statement. Wouldn't you
need to know if Query() succeeded before attempting to fetch result records?
> No, that's my point exactly.  Consider the following query:
Oops, misread that.  The point is that Query() succeeding does not guarantee that all
calls to Next() on the result will succeed.  Even without the scenario I described
above, what if the first row sent from the database does not conform to the protocol? 
What if it has garbage data in it?
To guarantee that Next() never returns an error (other than io.EOF) you would have to
fetch all the rows from the cursor, materialize them in the driver and check that the
data is as expected.  Which is a completely silly requirement to impose on all the
drivers just because QueryRow() ignores errors.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 26, 2013

Comment 10 by marko@trustly.com:

I'm also not a fan of how the return value of r.rows.Close() is also ignored.  That
could mask genuine errors as well, though it seems significantly less likely.
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Oct 26, 2013

Comment 11:

@marko, perhaps, you are right. I won't argue.
Alex
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 30, 2013

Comment 12 by marko@trustly.com:

Is there anything I can do to push this forward?  Attached is a patch implementing what
I think this code should look like.

Attachments:

  1. queryrow.patch (1387 bytes)
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Nov 1, 2013

Comment 13 by marko@trustly.com:

Ping?
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 1, 2013

Comment 14:

Please see http://golang.org/doc/contribute.html and feel free to send out the
codereview ("hg mail nnnn") once Go 1.2 is released.  We're currently in a freeze (see
http://golang.org/s/releasesched)

Labels changed: added go1.3, removed priority-triage.

Status changed to Accepted.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Nov 1, 2013

Comment 15 by marko@trustly.com:

This is strictly a bug fix, so I don't see how the feature freeze is relevant.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 1, 2013

Comment 16:

Sorry, code freeze, not feature freeze.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Nov 2, 2013

Comment 17 by marko@trustly.com:

Well I'm not sure why we would still have to push this to 1.3 instead of 1.2.1.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 2, 2013

Comment 18:

Only critical bug fixes that are regressions, well understood, and have good tests are
going in right now.  This bug is none of those three.
Sorry this didn't make it in, but we can't hold up every release for every bug, or we'd
never ship anything.  And there's a non-zero chance that this fix (which has no tests)
introduces yet another bug.  It's happened many times before.
@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 19:

Labels changed: added release-go1.3.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 20:

Labels changed: removed go1.3.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 21:

Labels changed: added repo-main.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 16, 2013

Comment 22:

This issue was closed by revision 1f20ab1.

Status changed to Fixed.

@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@rsc rsc removed the release-go1.3 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 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
4 participants
You can’t perform that action at this time.