database/sql: Erroneous ErrNoRows when using QueryRow("INSERT INTO ... RETURNING ...") when violating database constraint. #6651

gopherbot · 2013-10-23T21:17:56Z

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:

known_issues_test.go (1191 bytes)

alexbrainman · 2013-10-24T01:32:24Z

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 · 2013-10-24T10:37:05Z

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 · 2013-10-24T22:49:03Z

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:

fakedriver.go (758 bytes)
ignored.go (225 bytes)

alexbrainman · 2013-10-25T01:19:28Z

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 · 2013-10-25T07:00:54Z

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 · 2013-10-26T03:07:55Z

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 · 2013-10-26T07:44:15Z

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 · 2013-10-26T07:56:44Z

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 · 2013-10-26T11:51:05Z

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 · 2013-10-26T13:59:00Z

Comment 11:

@marko, perhaps, you are right. I won't argue.
Alex

gopherbot · 2013-10-30T16:50:47Z

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:

queryrow.patch (1387 bytes)

gopherbot · 2013-11-01T18:20:18Z

Comment 13 by marko@trustly.com:

Ping?

bradfitz · 2013-11-01T18:27:08Z

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 · 2013-11-01T18:29:55Z

Comment 15 by marko@trustly.com:

This is strictly a bug fix, so I don't see how the feature freeze is relevant.

bradfitz · 2013-11-01T18:31:02Z

Comment 16:

Sorry, code freeze, not feature freeze.

gopherbot · 2013-11-02T01:04:14Z

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 · 2013-11-02T02:03:31Z

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 · 2013-12-04T01:30:06Z

Comment 19:

Labels changed: added release-go1.3.

rsc · 2013-12-04T01:30:27Z

Comment 20:

Labels changed: removed go1.3.

rsc · 2013-12-04T01:47:02Z

Comment 21:

Labels changed: added repo-main.

bradfitz · 2013-12-16T20:48:42Z

Comment 22:

This issue was closed by revision 1f20ab1.

Status changed to Fixed.

gopherbot added fixed labels Dec 16, 2013

rsc added this to the Go1.3 milestone Apr 14, 2015

rsc removed the release-go1.3 label Apr 14, 2015

golang locked and limited conversation to collaborators Jun 25, 2016

gopherbot added the FrozenDueToAge label Jun 25, 2016

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

database/sql: Erroneous ErrNoRows when using QueryRow("INSERT INTO ... RETURNING ...") when violating database constraint. #6651

database/sql: Erroneous ErrNoRows when using QueryRow("INSERT INTO ... RETURNING ...") when violating database constraint. #6651

gopherbot commented Oct 23, 2013

alexbrainman commented Oct 24, 2013

gopherbot commented Oct 24, 2013

gopherbot commented Oct 24, 2013

alexbrainman commented Oct 25, 2013

gopherbot commented Oct 25, 2013

alexbrainman commented Oct 26, 2013

gopherbot commented Oct 26, 2013

gopherbot commented Oct 26, 2013

gopherbot commented Oct 26, 2013

alexbrainman commented Oct 26, 2013

gopherbot commented Oct 30, 2013

gopherbot commented Nov 1, 2013

bradfitz commented Nov 1, 2013

gopherbot commented Nov 1, 2013

bradfitz commented Nov 1, 2013

gopherbot commented Nov 2, 2013

bradfitz commented Nov 2, 2013

rsc commented Dec 4, 2013

rsc commented Dec 4, 2013

rsc commented Dec 4, 2013

bradfitz commented Dec 16, 2013

database/sql: Erroneous ErrNoRows when using QueryRow("INSERT INTO ... RETURNING ...") when violating database constraint. #6651

database/sql: Erroneous ErrNoRows when using QueryRow("INSERT INTO ... RETURNING ...") when violating database constraint. #6651

Comments

gopherbot commented Oct 23, 2013

alexbrainman commented Oct 24, 2013

gopherbot commented Oct 24, 2013

gopherbot commented Oct 24, 2013

alexbrainman commented Oct 25, 2013

gopherbot commented Oct 25, 2013

alexbrainman commented Oct 26, 2013

gopherbot commented Oct 26, 2013

gopherbot commented Oct 26, 2013

gopherbot commented Oct 26, 2013

alexbrainman commented Oct 26, 2013

gopherbot commented Oct 30, 2013

gopherbot commented Nov 1, 2013

bradfitz commented Nov 1, 2013

gopherbot commented Nov 1, 2013

bradfitz commented Nov 1, 2013

gopherbot commented Nov 2, 2013

bradfitz commented Nov 2, 2013

rsc commented Dec 4, 2013

rsc commented Dec 4, 2013

rsc commented Dec 4, 2013

bradfitz commented Dec 16, 2013