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: sql.Result has problems that should be fixed / replaced #19055

Open
kardianos opened this issue Feb 13, 2017 · 14 comments
Open

database/sql: sql.Result has problems that should be fixed / replaced #19055

kardianos opened this issue Feb 13, 2017 · 14 comments
Assignees
Milestone

Comments

@kardianos
Copy link
Contributor

@kardianos kardianos commented Feb 13, 2017

I'm not proposing anything at this point. I'm just documenting various things I've come things I've come across that relate to sql.Result.

@mjibson has said:

the current problem i'm having with the database/sql API is that it's not possible to get intermediate non-rows returning results when sending multiple statements in a single string
for example given create table t (i int); drop table a; i'd like to get back 2 results instead of 1. the next result sets stuff assumes that a full rows object instead of optionally returning a sql.Result instead. i am modifying lib/pq to support this, but only if you don't use database/sql.

The other @mattn has said:

One of the motivation is rowid in oracle. rowid is not number, so go-oci8 doesn't work correctly for the go's LastInsertId. The second, we have to allocate/free the buffer to get result specific value for the pl/sql.

@tgulacsi do you have anything to add?

@kardianos kardianos self-assigned this Feb 13, 2017
@kardianos kardianos changed the title database/sql: investigate various issues with the the sql.Result encountered database/sql: investigate various issues with the the sql.Result Feb 13, 2017
@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017
@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented Mar 31, 2017

@mjibson You mentioned you were modifying lib/pq to support getting each result. Did you do this and if so could you point me to the code? I'd like to see the details. Supporting this seems really close to also supporting NOTICE messages too.

@mattn It seems like the best solution might be to have a result that returned an interface{} value, then you could call x.Result().(int64) or x.Result().(GUID) or x.Result().(string). Might be useful for DBs like CRDB where keys may more often by strings of bytes.

Can the Last Insert ID also be returned from non-exec queries? Is that also desirable?

@mjibson

This comment has been minimized.

Copy link
Contributor

@mjibson mjibson commented Mar 31, 2017

No, I haven't done this yet, although I would still like to. The current plan is to add an internal method with a flag to return these intermediate messages or not during calls to NextResultSet.

@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented Mar 31, 2017

Throwing out ideas here. What if there was a new method attached to sql.Rows that could be called after sql.Next but before sql.NextResultSet that would return an array of interfaces where you can do a type switch to return various items, such as informational NOTICE messages, last inserted ID, rows affected.

for {
    for rows.Next() {
        rows.Scan(...)
    }
    for {
        switch m := rows.Message().(type) {
        case sql.RowsAffected:
           fmt.Println("affected:", v.Count)
        case sql.Notice:
            fmt.Println("NOTICE:", v.Message)
        case sql.LastInsertID:
            fmt.Println("ID:", v.Value.(GUID)) // Support more than just int64.
    }
    if !rows.NextResultSet() {
        break
    }
}

Buffered messages would be discarded after the next call to rows.NextResultSet or rows.Close.
Thoughts?

@mjibson

This comment has been minimized.

Copy link
Contributor

@mjibson mjibson commented Mar 31, 2017

Calling .Message like this presents some ordering problems. Say I have the multi-statement query:

insert into t values (1); select * from t; delete from t;

The above API would, as I read it, return a message only about the delete, but ignore the insert. That is, it seems like either all the messages before the first SELECT or after the last SELECT will be ignored with the above suggestion. You could decide that all the messages before the first SELECT get added to the Message calls for the first round, but that mixes the order of when they happened, which also seems wrong. I like some things about this suggestion, like it doesn't require yet another method on sql.DB, but I think it needs some more iteration. No ideas come to mind.

@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented Mar 31, 2017

Point taken. I'll think on it.

@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented Mar 31, 2017

@mjibson An alternative is that messages are cached until they are dequeued or rows.Close is called.
Thus you might do this:

handleMessages := func(rows *sql.Rows) {
    for {
        switch m := rows.Message().(type) {
        case sql.RowsAffected:
           fmt.Println("affected:", v.Count)
        case sql.Notice:
            fmt.Println("NOTICE:", v.Message)
        case sql.LastInsertID:
            fmt.Println("ID:", v.Value.(GUID)) // Support more than just int64.
    }
}
for {
    for rows.Next() {
        rows.Scan(...)
    }
    handleMessages(rows) // optional
    if !rows.NextResultSet() {
        break
    }
}
handleMessages(rows) // only need it here
rows.Close()

Perhaps the rows.Message() could be valid anytime before close and it would dequeue any pending messages received up till then.

It would be a bit awkward to call manually per query, but super easy in any type of framework / generic table buffer situation.

@mjibson

This comment has been minimized.

Copy link
Contributor

@mjibson mjibson commented Mar 31, 2017

Another choice is to have .Message be able to also indicate a rows result:

Loop:
	for {
		switch m := rows.Message().(type) {
		case sql.RowsAffected:
			fmt.Println("affected:", m.Count)
		case sql.Notice:
			fmt.Println("NOTICE:", m.Message)
		case sql.LastInsertID:
			fmt.Println("ID:", m.Value.(GUID)) // Support more than just int64.
		case sql.RowsResult:
			for rows.Next() {
				rows.Scan(&v)
			}
		default:
			if !rows.NextResultSet() {
				break Loop
			}
		}
	}

Something like this would preserve full statement ordering, which is a requirement for my use case.

@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented Mar 31, 2017

I like where this is going. We are only adding a single method, but allow a new way of consuming in sequence order all messages from the database, not just data results. It is also by nature is opt in.

Furthermore the message buffer should be able to be reused easily. I think if the RowsResult message was also used, then both rows.NextResultSet and rows.Close would clear any enqueued messages.

If you're happy with that, I'll see if I can work up a CL.

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Apr 2, 2017

I think rows.Message() is good for us to extend next feature.

@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented Apr 3, 2017

Please review https://go-review.googlesource.com/c/39355/ for API suitability and correctness.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 3, 2017

CL https://golang.org/cl/39355 mentions this issue.

@infogulch

This comment has been minimized.

Copy link

@infogulch infogulch commented Apr 4, 2017

This would work for my use case as well.

One thing I would proceed cautiously on is buffering of messages that aren't read. Some database procedures can produce a large number of messages. Right now I believe these are discarded by drivers. Existing clients may rely on the fact that these messages are discarded, but if this behavior changes underneath them I'm worried that this could lead to unbounded memory consumption in the driver waiting for the user to read such messages.

My point is we might want to consider making the usage of this feature more opt-in, specifically for memory consumption. Perhaps: stop buffering if a call to rows.Next() is made before any call to rows.Message.

@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented May 30, 2017

In go1.9, drivers will be able to accept non-query parameters as arguments to a query. I think this can be solved with https://github.com/golang-sql/sqlexp/blob/master/messages.go if a driver supports it.

I'm removing the milestone because I think this can be accomplished for now out of the std lib for now. If the approach works, we might want to bless this method in some way.

@kardianos kardianos removed this from the Go1.9Maybe milestone May 30, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Apr 13, 2018
@kardianos

This comment has been minimized.

Copy link
Contributor Author

@kardianos kardianos commented Apr 19, 2018

Currently as of Go1.10, results from an Exec are wrapped in the driverResult. Around each call to LastInsertId and RowsAffected it locks with the driverConn. There are fundamental problems with this design.

  1. You cannot design a correct driver that calls back into the database from the result because another insert may have happened in the same session/connection after the result was returned but before result.LastInsertId was called. The driver must ensure that it has the last insert ID prior returning initially.
  2. The driverResult locks around the driverConn, but if that has already been returned to the pool and may already be in use again. It could have a fundamentally different value on that connection or it may just block needlessly until that unrelated query is done and it can fetch the stored integer ID.
  3. Lastly, it assumes an IDENTITY column types (PostgreSQL and Oracle have named serials often) and that the IDENTITY is an integer. This is not always true.

We should fix these issues in Go2 #22697 .

@kardianos kardianos changed the title database/sql: investigate various issues with the the sql.Result database/sql: sql.Result has problems that should be fixed / replaced Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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