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

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

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

muhlemmer opened this issue Nov 24, 2019 · 10 comments

Comments

@muhlemmer
Copy link

@muhlemmer 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

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Author

@muhlemmer muhlemmer commented Nov 27, 2019

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 Dec 4, 2019
@rsc rsc modified the milestones: Backlog, Proposal Dec 4, 2019
@muhlemmer

This comment has been minimized.

Copy link
Author

@muhlemmer muhlemmer commented Dec 7, 2019

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 7, 2019

@muhlemmer Not at this time. Thanks.

@kardianos Any thoughts on this? Thanks.

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos 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 was marked as resolved.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 10, 2019

@kardianos When you wrote "continence method" did you mean "convenience method"?

@kardianos

This comment was marked as resolved.

Copy link
Contributor

@kardianos kardianos commented Dec 10, 2019

@ianlancetaylor Yes, I intended to write "convenience method" and is in no way a commentary upon my personal continence; though given that I confused the two, may shed doubt upon my cognizance.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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 Dec 11, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 8, 2020

No final comments, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 10, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.