No way to see an Insert error from QueryRow()

[chris-baynes]

No way to see an Insert error from QueryRow()

[leehambley]

Just for the record, here's some code from @chris-baynes to make this a little bit more comfortable, reproduced here incase the Gist disappears:

// similar to sql.Row but without the error field
type DbRowFromInsert struct {
    rows *sql.Rows
}

// Using sql.Row.Scan() as a template for this method, but modifying the errors.
func (row *DbRowFromInsert) Scan(dest ...interface{}) error {
    defer row.rows.Close()
    row.rows.Next()

    // There may be no rows, but Scan anyway to get the errors...
    // If there are no rows because of a db constraint error this is when those errors will be returned.
    err := row.rows.Scan(dest...)
    if err != nil {
        return err
    }

    return nil
}

// Usage:
func InsertReturning(query string, args ...interface{}) (*DbRowFromInsert, error) {
    rows, err := dbConn.Query(query, args...)
    if err != nil {
        return nil, err
    }

    return &DbRowFromInsert{rows: rows}, nil
}

[chris-baynes]

I've had a look through the code and, from my understanding, think this would be possible in the following way.
Exec would allow INSERT ... RETURNING queries. Either the query returns a single row and single column (which is then set on result lastInsertId), or no result rows are returned, or an error is set.

This would mean some changes to the result struct, and require handling of result rows in Exec. Do you think this is a feasible/sensible approach?

[fdr]

Interesting. I'll have to do some digging, I wonder if some database/sql mandated behavior is masking over any errors propagated from Postgres, since no rows in result set doesn't look like a Postgres-derived error to me.

[kisielk]

@chris-baynes What's wrong with using Exec()? The Result type has a LastInsertId() method that should give you the result from the INSERT ... RETURNING queries, or am I not understanding?

The Result type or Exec signature in the go standard library is not going to change because of the Go API guarantee.

[chris-baynes]

I've figured out a solution to this. Instead of using QueryRow(), I'm using Query() and getting the first row. I actually use a struct similar to sql.Row to do the work for me. Here's how it looks:
https://gist.github.com/chris-baynes/5361728

@kisielk As I already mentioned, Exec() does not return any rows, and hence no id from the insert if everything went well. LastInsertId() is not implemented in this package.

[jagregory]

I'm having the same issue.

@kisielk LastInsertId doesn't appear to be implemented in pq. I can use Exec and it'll give me the correct error when one occurs, but when everything goes ok LastInsertId errors with Error: no LastInsertId available.

If you look at the various Exec methods, none of them seem to set the Result return value.

func (st *stmt) Exec(v []driver.Value) (res driver.Result, err error) {
    defer errRecover(&err)

    if len(v) == 0 {
        return st.cn.simpleQuery(st.query)
    }
    st.exec(v)

    for {
        t, r := st.cn.recv1()
        switch t {
        case 'E':
            err = parseError(r)
        case 'C':
            res = parseComplete(r.string())
        case 'Z':
            // done
            return
        case 'T', 'N', 'S', 'D':
            // Ignore
        default:
            errorf("unknown exec response: %q", t)
        }
    }

    panic("not reached")
}

[kisielk]

@jagregory Yes, that was a misunderstanding on my part. There is no reasonable way to implement LastInsertId in pq.

[kisielk]

I think I see why this may not work with QueryRow correctly.

The high level logic going on in database/sql is something like:

rows, err := db.Query("...")
if err != nil {
    return err
}
if !rows.Next() {
    return ErrNoRows
}
err := rows.Scan(val)
return err

I haven't verified yet, but I think in the case of an InsertError pq will not tell you about it until you call rows.Err, which happens after rows.Next returns false, by which point QueryRow has already returned ErrNoRows.

[kisielk]

Have you tried go tip? I can't reproduce this there.

func TestInsertError(t *testing.T) {
    db := openTestConn(t)
    defer db.Close()

    _, err := db.Exec("CREATE TEMP TABLE test (i SERIAL PRIMARY KEY, v text)")
    if err != nil {
        t.Fatal(err)
    }
    r := db.QueryRow("INSERT INTO test VALUES (DEFAULT, 'hello') RETURNING i")
    var i int
    err = r.Scan(&i)
    if err != nil {
        t.Fatal(err)
    }
    if i != 1 {
        t.Fatalf("bad value for i: got %d, want 1", i)
    }

    r = db.QueryRow("INSERT INTO test VALUES (1, 'world') RETURNING i")
    err = r.Scan(&i)
    if err == nil {
        t.Fatal("expected an error")
    }

    _, ok := err.(PGError)
    if !ok {
        t.Fatalf("expected PGError, got: %#v", err)
    }
    t.Log(err)
}

The log message at the bottom gives:

conn_test.go:444: pq: S:"ERROR" C:"23505" M:"duplicate key value violates unique constraint \"test_pkey\"" D:"Key (i)=(1) already exists." F:"nbtinsert.c" L:"396" R:"_bt_check_unique"

which is as expected.

[kisielk]

Actually even looking at the code for Row.Scan in tip, it hasn't changed much since 1.1. Do you have some code you can share that reproduces the problem? It seems to work fine in the example I pasted above.

[jagregory]

I'll investigate some more, thanks for taking the time to look into it. I'm on Go 1.1.2. When I tried it earlier, I was getting sql.ErrNoRows instead of the constraint error.

[jagregory]

Ah, this was my mistake. I was running a version of pq from a few weeks ago. It looks like somebody has fixed it up since then! Thanks for the help.

[kisielk]

Cool. I added a test in 6cc5689 that should catch any regressions in this.

[leehambley]

I'm currently using master at 7876026 and seeing a regression here, I think.

I know there's a test case added that should guard against this regression, but this raises on the second insert for me, returning sql.ErrNoRows

// Inserting a row with RETURNING and violating a unique, or foreign key
// constraint should return the *pq.Error, not sql.ErrNoRows, I believe
// the root cause of this issue is inside the database/sql package itself
// as it counts the returned rows before checking for errors returned.
//
// http://golang.org/src/pkg/database/sql/sql.go around line #1424
//
// I had seen mention of this issue somewhere in the lib/pq issue tracker
// and a link to a Gist with a workaround, as well as some discussion about
// how eaigner\s hood ORM had worked around this.
func Test_RaisingPqErrorOnQueryRowConstraintViolation(t *testing.T) {

  var returned string

    db := openTestConn(t)
    defer db.Close()

    _, err := db.Exec("CREATE TEMP TABLE pqerronviolateconstraint (a varchar(3) not null UNIQUE)")
    if err != nil {
        t.Fatal(err)
    }

  err = db.QueryRow("INSERT INTO pqerronviolateconstraint(a) values($1) RETURNING a", "abc").Scan(&returned)
    if err != nil {
        t.Fatal(err)
    }

  err = db.QueryRow("INSERT INTO pqerronviolateconstraint(a) values($1) RETURNING a", "abc").Scan(&returned)
    if err == sql.ErrNoRows {
        t.Fatal(err)
    }

}

[leehambley]

The full results of $ go test [-v] included below for completeness:

➜  pq git:(master) ✗ go test
Skipping failing test: see https://github.com/lib/pq/issues/148
--- FAIL: Test_RaisingPqErrorOnQueryRowConstraintViolation (0.01 seconds)
    known_issues_test.go:66: sql: no rows in result set
FAIL
exit status 1
FAIL    _/Users/leehambley/Projects/leehambley/pq   0.304s
➜  pq git:(master) ✗ go test -v
=== RUN TestDecodeBool
--- PASS: TestDecodeBool (0.01 seconds)
=== RUN TestReconnect
--- PASS: TestReconnect (0.04 seconds)
=== RUN TestOpenURL
--- PASS: TestOpenURL (0.01 seconds)
=== RUN TestExec
--- PASS: TestExec (0.02 seconds)
=== RUN TestStatment
--- PASS: TestStatment (0.04 seconds)
=== RUN TestRowsCloseBeforeDone
--- PASS: TestRowsCloseBeforeDone (0.01 seconds)
=== RUN TestEncodeDecode
--- PASS: TestEncodeDecode (0.01 seconds)
=== RUN TestNoData
--- PASS: TestNoData (0.01 seconds)
=== RUN TestError
--- PASS: TestError (0.05 seconds)
=== RUN TestBadConn
--- PASS: TestBadConn (0.00 seconds)
=== RUN TestErrorOnExec
--- PASS: TestErrorOnExec (0.01 seconds)
=== RUN TestErrorOnQuery
--- PASS: TestErrorOnQuery (0.02 seconds)
=== RUN TestErrorOnQueryRow
--- PASS: TestErrorOnQueryRow (0.02 seconds)
=== RUN TestSimpleQuery
--- PASS: TestSimpleQuery (0.02 seconds)
=== RUN TestBindError
--- PASS: TestBindError (0.02 seconds)
=== RUN TestReturning
--- PASS: TestReturning (0.03 seconds)
=== RUN TestParseEnviron
--- PASS: TestParseEnviron (0.00 seconds)
=== RUN TestExecerInterface
--- PASS: TestExecerInterface (0.00 seconds)
=== RUN TestNullAfterNonNull
--- PASS: TestNullAfterNonNull (0.01 seconds)
=== RUN Test64BitErrorChecking
--- PASS: Test64BitErrorChecking (0.02 seconds)
=== RUN TestRollback
--- PASS: TestRollback (0.03 seconds)
=== RUN TestParseOpts
--- PASS: TestParseOpts (0.00 seconds)
=== RUN TestRuntimeParameters
--- PASS: TestRuntimeParameters (0.14 seconds)
=== RUN TestIsUTF8
--- PASS: TestIsUTF8 (0.00 seconds)
=== RUN TestXactMultiStmt
--- SKIP: TestXactMultiStmt (0.00 seconds)
    conn_xact_test.go:12: Skipping failing test
=== RUN TestScanTimestamp
--- PASS: TestScanTimestamp (0.00 seconds)
=== RUN TestScanNilTimestamp
--- PASS: TestScanNilTimestamp (0.00 seconds)
=== RUN TestParseTs
--- PASS: TestParseTs (0.00 seconds)
=== RUN TestTimestampWithTimeZone
--- PASS: TestTimestampWithTimeZone (0.05 seconds)
=== RUN TestTimestampWithOutTimezone
--- PASS: TestTimestampWithOutTimezone (0.02 seconds)
=== RUN TestStringWithNul
--- PASS: TestStringWithNul (0.01 seconds)
=== RUN TestByteToText
--- PASS: TestByteToText (0.01 seconds)
=== RUN TestNo148
Skipping failing test: see https://github.com/lib/pq/issues/148
--- PASS: TestNo148 (0.00 seconds)
=== RUN Test_RaisingPqErrorOnQueryRowConstraintViolation
--- FAIL: Test_RaisingPqErrorOnQueryRowConstraintViolation (0.01 seconds)
    known_issues_test.go:66: sql: no rows in result set
=== RUN TestSimpleParseURL
--- PASS: TestSimpleParseURL (0.00 seconds)
=== RUN TestFullParseURL
--- PASS: TestFullParseURL (0.00 seconds)
=== RUN TestInvalidProtocolParseURL
--- PASS: TestInvalidProtocolParseURL (0.00 seconds)
=== RUN TestMinimalURL
--- PASS: TestMinimalURL (0.00 seconds)
FAIL
exit status 1
FAIL    _/Users/leehambley/Projects/leehambley/pq   0.639s

[kisielk]

I think ErrNoRows is correct in this case, from the Postgres docs:

If the INSERT command contains a RETURNING clause, the result will be similar to that of a SELECT statement containing the columns and values defined in the RETURNING list, computed over the row(s) inserted by the command.

Since the query doesn't actually insert anything because of the unique constraint, there are no rows over which RETURNING is computed.

[leehambley]

Sure, but when implemented in terms of an *[sql,tx].Exec, with a *sql.Result returned, there's an error returned by the database, specifically, I think the case in database/sql should be:

1. Check for errors, return if any.
2. Try and read a row. (or return sql.ErrNoRows, which is where this process starts, currently)
3. Return error if there's a problem reading a row.

It seems like the Go driver checks for no rows, before checking if there was an error with the query, which in theory, is backwards, as they get an error result, but don't check it until after examining the (possibly bogus) rows object they get back... at least that's my take. Probably the subtle database semantics between MySQL and PostgreSQL etc don't really apply here, and I will just have to wrap this in my code to make it less ugly.

Given the complexity, I was really hoping to lean on QueryRow rather than having to do all the complex error checking (err on query?, check for rows, error on checking rows?)

I'm also curious as to why the TestErrorOnQueryRow test case passes, as I suppose that also returns no rows, but you are explicitly raising the error there, whereas the error is implicit in my example code.

[kisielk]

Hm yes, if Query is returning an error in this case, then QueryRow probably should too. It's probably too late to fix in Go 1.2 but you could file an issue in the Go tracker.

[leehambley]

Right, if I do open an issue, I'll mention it here for posterity, thanks for the second opinion. Time to code a workaround!

[johto]

I'm also curious as to why the TestErrorOnQueryRow test case passes, as I suppose that also returns no rows, but you are explicitly raising the error there, whereas the error is implicit in my example code.

The difference is probably that TestErrorOnQueryRow uses the simpleExec path, whereas your code does a full prepare/execute.

[johto]

FWIW, this looks like a bug in database/sql to me. What it does it this:

    if !r.rows.Next() {
        return ErrNoRows
    }

And Rows.Next() looks like this:

// Next prepares the next result row for reading with the Scan method.
// It returns true on success, false if there is no next result row.
// Every call to Scan, even the first one, must be preceded by a call
// to Next.
func (rs *Rows) Next() bool {
    if rs.closed {
        return false
    }
    if rs.lasterr != nil {
        return false
    }
    if rs.lastcols == nil {
        rs.lastcols = make([]driver.Value, len(rs.rowsi.Columns()))
    }
    rs.lasterr = rs.rowsi.Next(rs.lastcols)
    if rs.lasterr == io.EOF {
        rs.Close()
    }
    return rs.lasterr == nil
}

So not checking rs.lasterr after calling Next() looks to be the cause of this, and a pretty obvious bug if you ask me.

[leehambley]

Do either of you guys have any experience filing bugs with the Go issue tracker, I wonder how they'd respond to being told that that there's extensive discussion about it at Github?!

You're right, though they should switch on the result of rs.lasterr = rs.rowsi.Next(rs.lastcols) 6 lines from the bottom, I guess, otherwise they miss when the database has thrown a constraint violation error.

[kisielk]

I've filed lots of issues there. It's best to just put the relevant points in the issue description.

[johto]

I've filed lots of issues there. It's best to just put the relevant points in the issue description.

Would you mind filing a report? It would be really great to get this fixed in 1.2 if possible, and I think you're in a better time zone for that than I am.

[kisielk]

Maybe @leehambley can do it? He seems to be most familiar with the details and I'd rather not have to act as an intermediary :)

[leehambley]

Done, issue filed at https://code.google.com/p/go/issues/detail?id=6651&thanks=6651&ts=1382563078

[GorgeousPuree]

I've just faced the same problem. Solution described here https://gist.github.com/chris-baynes/5361728 is nice, but currently I'm trying to write SQL-transaction with RETURNING query and I'm stuck with it.
This is an example of SQL-transactions which I tried to use https://pseudomuto.com/2018/01/clean-sql-transactions-in-golang/ and an Exec() method was used there. So same story, but my knowledge doesn't allow me to implement something like in the first link. And I'm looking for help, please.