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 · 15 comments
Open

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

kardianos opened this issue Feb 13, 2017 · 15 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@kardianos
Copy link
Contributor

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 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 13, 2017
@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
Copy link
Contributor Author

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

@maddyblue
Copy link
Contributor

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
Copy link
Contributor Author

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?

@maddyblue
Copy link
Contributor

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
Copy link
Contributor Author

Point taken. I'll think on it.

@kardianos
Copy link
Contributor Author

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

@maddyblue
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

mattn commented Apr 2, 2017

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

@kardianos
Copy link
Contributor Author

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

@gopherbot
Copy link

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

@infogulch
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
@billgraziano
Copy link

Something like this has my vote. I'm struggling with this right now.

I do want to make sure we think about something like BACKUP that doesn't return result sets but returns lots of messages while it's running. It seems like that would be handled.

I'd even be happy if I could just pass in a logger of some type. The two options I'm thinking about now are to hack "sql" to print these out in a format I like or wrap sqlcmd.exe and just print all the results. I don't really like either :-)

I also thought about sending in a channel to "sql" that I could consume. I'm afraid that has problems I haven't discovered though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants