I'll accept there is a documentation issue here at the least and there is possibly a reasonable code change..
The related issue is #24329
And comment in issue here: #23738 (comment)

Right now if rows.Scan produces an error, it isn't a good idea to necessarily just return that error and run. You'd really want to:

1. Check Rows.Scan, assume err != nil.
2. Check Rows.Err
3. If Rows.Err == nil, call Rows.Close and return Rows.Scan's error.
4. If Rows.Err != nil, return that error.

If Rows.Scan does not return an error, you may safely omit the Rows.Err check, but you shoud still check for an error from Rows.Close. Let's see what that would look like:

for rows.Next() {
    var id int64
    err := rows.Scan(&id)
    if err != nil {
        if rows.Err() != nil {
            return rows.Err()
        }
        rows.Close()
        return err
    }
}
if err := rows.Close(); err != nil {
    return err
}
// Do work.

or

var scanErr error
for rows.Next() {
    var id int64
    scanErr = rows.Scan(&id)
    if scanErr != nil {
        break
    }
}
if rows.Err() != nil {
    return rows.Err()
}
if err := rows.Close(); err != nil {
    return err
}
if scanErr != nil {
    return scanErr
}
// Do work.

With your proposal you could write:

for rows.Next() {
    var id int64
    err := rows.Scan(&id)
    if err != nil {
        rows.Close() // We may be able to ignore this error.
        return err // This would be either a scan error or Rows.Err(lasterr) from Rows.Next.
    }
}
if err := rows.Close(); err != nil {
    return err
}
// Do work.

It does seem like it could simplify some code, or make existing code actually more useful. The downside someone may be (probably is) checking the text of rows.Scan's error and doing something with it. So this may be a compatibility issue.

(Note, I would only be concerned about the close error if I was writing to the database while also reading from it.)

I'd assume that calling Scan would always return a syntax/parsing/type casting failure from reading the value, and I might rewrap in a different way than I'd wrap rows.Err or rows.Close, which I would expect to be a connection failure or timeout.

One of the motivations for this is that in the case of Row, there isn't an option to check Close or Err from the underlyingRows. To catch the context close error would require this (or something similar):

err := db.QueryRowContext(ctx, q).Scan(&id)
if err.Error() == "sql: Rows are closed" && ctx.Err() != nil {
    return ctx.Err()
} else if err != nil {
    return err
}

Which could just be simplified to:

err := db.QueryRowContext(ctx, q).Scan(&id)
if err != nil {
    return err
}

Rows Next->Scan->Err seems to have been built with a single-threaded use in mind, but with the additional of context timeouts and cancellations, other errors can happen between the successive calls. Scan already returns an error (sql: Rows are closed), but sometimes the error could be more specific or give the cause.

@erykwalder Your last comment isn't convincing; Row.Scan checks for Rows.Err and Rows.Scan and Rows.Close errors all in one.

But I agree with you in general, I really see no reason not to return the lasterr if present on scan if Rows is closed.

@bradfitz I don't think this will be a compatibility problem, but theoretically someone could be checking the error text from rows.Scan. Do you have an opinion on this? If you don't I lean to doing this.

@erykwalder Your last comment isn't convincing; Row.Scan checks for Rows.Err and Rows.Scan and Rows.Close errors all in one.

It does, but again, does not handle errors that can happen asynchronously or from separate routines (specifically a context being timed out or cancelled). This is demonstrated by a simple program which runs inconsistently:

package main

import (
	"context"
	"database/sql"
	"fmt"
	"time"

	_ "github.com/lib/pq"
)

func main() {
	db, err := sql.Open("postgres", "postgres:///test?sslmode=disable")
	if err != nil {
		panic(err)
	}

	ctx, cancel := context.WithTimeout(context.Background(), 50*time.Microsecond)
	defer cancel()

	err = db.QueryRowContext(ctx, `SELECT 1`).Scan(new(int))
	if err != nil && err != context.DeadlineExceeded {
		fmt.Println(err)
	}
}

With sufficient runs, it will print sql: Rows are closed.

Looking at the source of Row.Scan, the problem is identifiable (comments are mine):

func (r *Row) Scan(dest ...interface{}) error {
	if r.err != nil {
		return r.err
	}

	defer r.rows.Close()
	for _, dp := range dest {
		if _, ok := dp.(*RawBytes); ok {
			return errors.New("sql: RawBytes isn't allowed on Row.Scan")
		}
	}

	if !r.rows.Next() {
		if err := r.rows.Err(); err != nil {
			return err
		}
		return ErrNoRows
	}
	// lastErr updated if context is closed by another routine here
	// won't be caught or returned
	err := r.rows.Scan(dest...)
	if err != nil {
		return err
	}
	if err := r.rows.Close(); err != nil {
		return err
	}

	return nil
}

@bradfitz I don't think this will be a compatibility problem, but theoretically someone could be checking the error text from rows.Scan. Do you have an opinion on this? If you don't I lean to doing this.

Let's try this and test it over the next 4 months and see.

I think making Scan return the correct errors on context cancels makes sense. Returning "Rows are closed" when they were really just canceled is slightly misleading, even if they were closed by the context-cancel-waiting goroutine.

@kardianos I'm happy to work on this if you don't already have something. I've already submitted a CLA fwiw.

@erykwalder That would be fine with me.

Change https://golang.org/cl/104276 mentions this issue: database/sql: return context errors from Rows.Scan