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: add error getter on sql.Row #35804

Closed
muhlemmer opened this issue Nov 24, 2019 · 10 comments
Closed

database/sql: add error getter on sql.Row #35804

muhlemmer opened this issue Nov 24, 2019 · 10 comments

Comments

@muhlemmer
Copy link
Contributor

muhlemmer commented Nov 24, 2019

What version of Go are you using (go version)?

$ go version
go version go1.13.4 linux/amd64

This is a small feature request.

go/src/database/sql/sql.go

Lines 3093 to 3097 in 6f7b96f

type Row struct {
// One of these two will be non-nil:
err error // deferred error for easy chaining
rows *Rows
}

At this moment QueryRow() variants only return a row, and as mentioned in the above comments, for easy chaining. This makes it a bit inconvenient to program against QueryRow() if one wants to check the error and return the Row as-is to a higher level caller. For instance, in my usecase I would like to wrap QueryRow() in go routines against multiple hosts and return the first successful result.

Would it be possible to have an error getter method (eg. row.Err()) on the the sql.Row type? This way "chaining" is still possible, while giving also more control over the error checking mechanism.

Edit: forgot to mention that I'm willing to implement this in a PR myself.

@toothrot toothrot changed the title Feature: Error getter on sql.Row database/sql: Proposal: add error getter on sql.Row Nov 26, 2019
@toothrot toothrot added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Nov 26, 2019
@toothrot
Copy link
Contributor

Hi @muhlemmer, could you provide a code example of the chaining that this API change would improve?

/cc @bradfitz @kardianos

@toothrot toothrot added this to the Backlog milestone Nov 26, 2019
@odeke-em odeke-em changed the title database/sql: Proposal: add error getter on sql.Row proposal: database/sql: add error getter on sql.Row Nov 26, 2019
@muhlemmer
Copy link
Contributor Author

Hi @muhlemmer, could you provide a code example of the chaining that this API change would improve?

Example 1:

// Node represents a database server connection
type Node struct {
	db             *sql.DB
        // other stuff that keeps track of Node health statistics and re-connection parameters.
}

// CheckErr checks for "serious" errors, like connection and database consistency.
// For example: lib/pq has predefined error codes which can be checked.
// Serious errors are counted as failures.
// If a the configured failure trashhold is reached, this node will we disconnected.
func (n *Node) CheckErr(err error) error {
    // Call helper methods for checking and decision making
    return err  //return the original error
}

// QueryRowContext wrapper around sql.DB.QueryRow.
// Implements boil.ContextExecutor
func (n *Node) QueryRowContext(ctx context.Context, query string, args ...interface{}) *sql.Row {
        row, err := n.DB().QueryRowContext(ctx, query, args...)
	return rows, n.CheckErr(row.Err()) // <--- This is what I would like to do
}

Example 2 extends example 1:

// MultiNode holds a slice of Nodes.
// All methods on this type run their sql.DB variant in one Go routine per Node.
type MultiNode []*Node

// QueryRowContext runs sql.DB.QueryRowContext on the Nodes in seperate Go routines.
// The first non-error result is returned immediatly.
// Errors from remaining Nodes will not be returned,
// just logged for stastics and descison making.
//
// The following errors can be returned:
// - If all nodes respond with the same error, that exact error is returned as-is.
// - If there is a variaty of errors, they will be embedded in a MultiError return.
//
// Implements boil.ContextExecutor.
func (mn MultiNode) QueryRowContext(ctx context.Context, query string, args ...interface{}) (*sql.Row, error) {
	rc := make(chan *sql.Row, len(mn))
	for _, n := range mn {
		go func(n *Node) {
			rc <- n.QueryRowContext(ctx, query, args...)
		}(n)
	}

	var me MultiError
	for i := 0; i < len(mn); i++ {
		row := <-rc
		if err := row.Err(); err == nil { // <-- Same here: this is what I would like to do
			return row, nil
		} else {
			me.append(err)
		}
	}
	return nil, me.check() // Compare errors and return accordingly.
}

I would like both the Node and MultiNode interface compatible with sql.DB and sql.Tx. Or more specifically with this interface:

// ContextExecutor can perform SQL queries with context
type ContextExecutor interface {
	Executor


	ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error)
	QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error)
	QueryRowContext(ctx context.Context, query string, args ...interface{}) *sql.Row
}

So that I can pass this types as DB or Tx connetion objects to the ORM.

Ultimately, a caller of my package's objects could replace:

var int i
err := db.QueryRowContext(ctx, "select $1", 1).Scan(&i)

With:

var int i
err := nodes.QueryRowContext(ctx, "select $1", 1).Scan(&i)

And keep the same chaining as the database/sql package currently provides, while my package can inspect the error in any given moment.

@rsc rsc added this to Incoming in Proposals (old) Dec 4, 2019
@rsc rsc modified the milestones: Backlog, Proposal Dec 4, 2019
@muhlemmer
Copy link
Contributor Author

I see there is still a WaitingForInfo tag on this issue. Is there anything else you would like me to clarify?

@ianlancetaylor ianlancetaylor removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 7, 2019
@ianlancetaylor
Copy link
Contributor

@muhlemmer Not at this time. Thanks.

@kardianos Any thoughts on this? Thanks.

@kardianos
Copy link
Contributor

kardianos commented Dec 9, 2019

*Row is a struct so adding a new method would be fine, and adding Err() error would be symmetric to *Rows. I see no technical reason not to add it.

The benefit I would cite to adding it would be it allows you to differentiate query errors from Scan errors and in a framework act appropriately to each.

The two strikes against this proposal is that (1) it makes this less of a convenience method (expands use case rather then just using *Result for most use cases) and (2) I tend to make different types of abstractions then what is presented here.

I'm fine with adding this.

@ianlancetaylor

This comment has been minimized.

@kardianos

This comment has been minimized.

@rsc
Copy link
Contributor

rsc commented Dec 11, 2019

Based on the discussion, this seems like a likely accept.

Leaving open for a week for final comments.

@rsc rsc moved this from Incoming to Likely Accept in Proposals (old) Dec 11, 2019
@rsc
Copy link
Contributor

rsc commented Jan 8, 2020

No final comments, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jan 8, 2020
@rsc rsc changed the title proposal: database/sql: add error getter on sql.Row database/sql: add error getter on sql.Row Jan 8, 2020
@rsc rsc modified the milestones: Proposal, Backlog Jan 8, 2020
@gopherbot
Copy link

Change https://golang.org/cl/214317 mentions this issue: database/sql: add error getter on sql.Row

@golang golang locked and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants