Prove two new go.19 CLs

[kardianos]

There are two new CLs in the works: https://go-review.googlesource.com/c/38533/ to support custom arguments (parameters) https://go-review.googlesource.com/c/39355/ to support a message loop / multiple row counts and msgs

Custom arguments should be easy. Just define a new method on the connection that verifies the driver type. If OUTPUT parameters are supported, the values should be inspected to be the correct type (pointers to value X) and they should be stored temporarily until the query returns and then set.

The message loop will probably be a little more complicated. In the Message method, want to make sure to not block after the QueryContext(ctx) context is canceled.

@mjibson Would you help create a branch for the postgresql driver for one or both of these in the next week? I'd like to get these CLs submitted, but I want to ensure we have at least one POC working. I plan to do the same with the mssql driver as well.

[maddyblue]

An update: I did some work on this but then later realized it was going to be a larger refactor. I have some ideas about how to proceed, but it may not come quickly.

[kardianos]

Thanks for the update. The go1.9 freeze is coming up quickly and I'd ideally have these in official review before then. If there is a way to have a POC by end of April, that would be great. I'm behind on the ms sql driver poc, but I hope to finish it this week.

[maddyblue]

Ok. I think I can get this done this week too.

[maddyblue]

Here's my current dilemma: the Message interface changes the API of the driver for its other calls. Take the following:

rows, err := db.Query("insert into t values (1); select 0/0")

In the go 1.8 API, this will return an error because the 0/0 in the SELECT produces an error.

However with the new message API being a possibility, the call to Query should not produce an error. Instead, we need to call rows.Message twice, returning a MsgRowsAffected and a MsgError. So we have two behaviors for the same input. I'm not sure what other sql drivers currently do but I wouldn't be surprised if it's creating the same problem with them too. It seems we may need to rethink the message API because it would require drivers changing behavior.

One idea is to add a method on sql.DB that produces a message object, and leave the rows alone.

[kardianos]

@mjibson You're right.

type RawMessage interface{}
type Messager struct {}
func (m *Messager) Message() RawMessage

// RawMessage falls back to using QueryContext if the message interface isn't supported.
func (db *DB) RawMessage(ctx context.Context, query string, args interface{}...) (*Rows, *Messager) {...}

func TestMessage(t *testing.T) {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()

    rows, rm := db.RawMessage(ctx, q)
    defer rows.Close()
    for {
         // Include ctx here so we don't store context from RawMessage.
        switch m := rm.Message(ctx).(type) {
        case error:
            t.Fatal(err)
        case driver.MsgNextResultSet:
            if !rows.NextResultSet() {
                return
            }
        case driver.MsgNext:
            for rows.Next() {
                rows.Scan(...)
            }
        case driver.MsgError:
            t.Log("SQL ERROR", m.Error)
        case driver.MsgNotice:
            t.Log("SQL NOTICE", m.Message)
        case driver.MsgRowsAffected:
            t.Log("SQL affected", m.Count)
        }
    }
}

The above would probably work. There will may be more or less pushback because we are no longer proposing adding a new method to rows, but are adding a new top level method to (DB, Tx, Stmt, Conn). I'm unsure what the driver API would look like in this scenario, but it could look like this:

type ConnMessage interface {
    Conn

    // MessageQueryContext is the same as QueryContext but it only returns initial transport
    // errors from the initial call. SQL errors are reported through Message.
    MessageQueryContext(ctx context.Context, query string, args []NamedValue) (Rows, error)

    Message() (RawMessage, error)
}

Feedback would be great. I'll need to look at this more in the morning. The difficulty is the existing semantics are less precise than what is offered by this call; you can construct the existing API from this call, but not the other way around. Probably the largest challenge to a driver supporting it is structural. I'll need to prototype it in the CL to see the effects in the database/sql code base; let me know if this looks like a good direction to go in.

[maddyblue]

Does it make sense to not return a rows at all? And instead implement Next, Scan, and NextResultSet on the message itself? A second option, if we aren't tied to a Rows at all, we can ditch NextResultSet altogether. The driver.MsgNext type could implement Next and Scan, and we add a driver.MsgClose type to indicate it's done. Returning both a rows and a message, then interleaving calls between them feels like an API with a bad amount of coupling, and complicated internal state handling.

[kardianos]

I like the idea of ditching NextResultSet in this case, but we would need to introduce a "MsgDone" as well. We would also want a Close, Columns, and ColumnTypes call, or equiv data returned.

func {(db *DB), (tx *Tx), (c *Conn)} RawMessage(ctx context.Context, query string, args ...interface{}) *Msg
func (s *Stmt) RawMessage(ctx context.Context, args ...interface{}) *Msg

type RawMessage interface{}

type Msg struct {...} // Contains unexported fields.

func (m *Msg) Message(ctx context.Context) RawMessage

// Close may be called at any time to close the current query. It doesn't need to be called after
// MsgDone is returned from Message.
func (m *Msg) Close() error

type (
    // MsgDone is the only message that must be checked for. Any fatal non-SQL error will be reported
    // in Error and should be checked before finishing the message loop.
    MsgDone struct { Error error}

    // MsgResultSet is called before each result set is available.
    // If Message is called after MsgResultSet is recieved without scanning the rows
    // the rows will be discarded and any next result set will be proceeded to be read.
    MsgResultSet struct{...} // contains unexported fields (probably contains a *Rows).

    // MsgError reports any SQL level errors delivered by the database to the client.
    // Multiple errors may be returned in a single response.
    MsgError struct{...}

    MsgNotice struct{...}
    MsgLastInsertID struct{ Value interface{} }
    MsgRowsAffected struct{ Count int64 }
)
func (m MsgResultSet) Next() bool
func (m MsgResultSet) Scan(dest ...interface{}) error
func (m MsgResultSet) Columns() ([]string, error)
func (m MsgResultSet) ColumnTypes() ([]*ColumnType, error)

And used like:

msg := db.RawMessage(ctx, "INSERT INTO Company (Name) values ('cono1'); SELECT * from Account;")
defer msg.Close()
for {
    switch m := msg.Message(ctx).(type) {
    case driver.MsgDone:
        return m.Error
    case driver.MsgResultSet:
        for m.Next() {
            err := m.Scan(...)
       }
    case ...
}

[kardianos]

@mjibson I'm finding the above is a huge API addition on a number of fronts.

What if we continued using the approach in https://go-review.googlesource.com/c/39355/ but if the last argument passed to the driver is type driver.ReturnMessage struct{} then the driver should enqueue messages internally and not return on SQL errors but expect a call to either Close or Message. Would that work?

[maddyblue]

That sounds ok, but I'm not sure what you mean by "last argument passed to the driver". Does that mean:

rows, err := db.Query("select 1; insert 2", driver.ReturnMessage{})

[kardianos]

Yeah, that's it exactly. But the user wouldn't pass it, database/sql would append it to the query arg list at the end, but only if it supports the message interface. That way drivers that support it can be signalled, hey, this is a message query, enqueue up messsages and change how errors are delivered.

[kardianos]

I'm not thinking well. Yeah, the user would need to pass it in. I wonder if rather than adding new sql API, we could pass in the driver type with the Message method on it that then gets registered.

Let me play with this, I'll update the CL later today or tomorrow.

[maddyblue]

Ok, so if I'm understanding this correctly a driver would register itself to support the Message stuff? That would mean users have to choose which version of the driver they want, because most drivers auto-register themselves during init().

[kardianos]

I was thinking the driver.Conn would need to implement an optional interface to tell database/sql it is supported. I'll have something concrete soon.

[kardianos]

@mjibson I've updated the CL https://go-review.googlesource.com/c/39355/ (but not the tests). I think that if https://golang.org/cl/38533 goes in this can be implemented completly outside of the std lib, probably under https://github.com/golang-sql/ .

What do you think of that approach? If that looks alright, I should focus on custom params in https://golang.org/cl/38533.

[kardianos]

@mjibson It also occurred to me that we could also pass in this ReturnMessage type into the context today and totally skip custom params and implement this in go1.8 today.

[maddyblue]

This approach looks pretty reasonable to me, and nicely avoids any massive changes to existing APIs. Is https://go-review.googlesource.com/c/39355/ good enough to cherry-pick now (along with https://go-review.googlesource.com/c/38533/, of course), so I can test it out? We definitely need to make sure this is implementable since the last time we tried we found problems in the proposed API.

[kardianos]

@mjibson I've implemented OUTPUT parameters in https://github.com/denisenkom/go-mssqldb/commit/0a7dca4ffb539829dd34bc34ee2d2e227e69f494#diff-fd31321fc6476abe1517235c03a0aa8cR141 in ms sql driver using cl/39355.

Right now I'm thinking to not use cl/38533 at all, but instead put those interfaces for now in an external repo https://github.com/golang-sql/sqlexp . I think the interfaces we care about can be cherry picked from those CLs to be used (we just will need to do a minor touch up to point to to sqlexp when I put it in there).

I've got to pick up a sick kid from school, but if you have any questions feel free to email/hangouts/voip me.

[kardianos]

I've updated the CL for argument parameters https://golang.org/cl/38533 and I'm very happy with the results there. I've put the message loop code here https://github.com/golang-sql/sqlexp/commit/c6fb99775facdd33d510f67669b3d8038014a8bc .

The main change for https://golang.org/cl/38533 (custom arguments) is the addition to be able omit or pass the checks on.

[kardianos]

The upshot is the custom param support with output variables went in, but the message loop didn't make it. I think the message loop can be implemented outside the std lib.