ziutek / mymysql

MySQL Client API written entirely in Go
Other
735 stars 161 forks source link

Multi result set, discard after first, for godrv for Stored Procedures #107

Open PuppyKhan opened 9 years ago

PuppyKhan commented 9 years ago

There are 2 changes here:

  1. Sprintf was missing some arguments
  2. Added a loop to get and discard all subsequent result sets after the first one is complete. This allows the driver to work with Stored Procedures.

Note: The import sorting is from using github.com/bradfitz/goimports.

ziutek commented 9 years ago

Can you provide a patch that sumarizes all changes, preferably in one or two commits? I tried to follow some first steps but it seems that this pull request contanis commits for all your tries.

PuppyKhan commented 9 years ago

Only the last one matters, Github automatically through the whole history in there. I'll redo it as 2 commits for the 2 separate pieces.

PuppyKhan commented 9 years ago

Squashed the commits so irrelevant changes removed from the commit history. Now it is two distinct changes, which should be much clearer.

lucalooz commented 9 years ago

I needed something like this so i looked into it, with this approach QueryRow will fail, you need to discard other result also inside the Close() function. I moved all the discard/close thing inside a private end() function like this:

func (r *rowsRes) end(close bool) error {
    if r.my == nil {
        return nil
    }
    if close {
        if err := r.my.End(); err != nil {
            return err
        }
    }
    // Stored Procedure hack for godrv: always ignore multi results
    nextRes, err := r.my.NextResult()
    for err != nil && nextRes != nil {
        if err := nextRes.End(); err != nil {
            return err
        }
        nextRes, err = nextRes.NextResult()
    }
    if err != nil {
        return err
    }
    if r.simpleQuery != nil && r.simpleQuery != textQuery {
        if err := r.simpleQuery.Delete(); err != nil {
            return err
        }
    }
    r.my = nil
    return err
}

func (r *rowsRes) Close() error {
    if err := r.end(true); err != nil {
        return errFilter(err)
    }
    return nil
}

func (r *rowsRes) Next(dest []driver.Value) error {
    if r.my == nil {
        return io.EOF // closed before
    }
    // ScanRow ...
    if err != io.EOF {
        return errFilter(err)
    }
    if err := r.end(false); err != nil {
        return errFilter(err)
    }
    return io.EOF
}
PuppyKhan commented 9 years ago

Finally had some time to run more thorough tests. You are correct @Idhor & it fails spectacularly. Working on a fix, but you should get contributor credit as well.

PuppyKhan commented 9 years ago

OK, I ran a bunch of tests on the latest version and it works for both Query, where the last row in Next effectively closes result so later call to Close does nothing, and works for QueryRow, where Scan calls Close internally.

The case not yet considered is the Exec method. With a simple SP not returning any rows, it works fine. But with multiple result sets, this breaks, and there seems to be no way to use a similar solution as the spec does not have any subsequent steps yet needs the result available. I am currently exploring an option requiring an extra user step.

For now, this solution works for both Query and QueryRow, be they called from the connection, prepared statement, of transaction. It neither helps nor hinders Exec.

(Edit: fixed typos)

ChuckByram commented 9 years ago

Hi @ziutek , I was wondering if this one had a chance of being merged. This addresses issues caused by an idle connection being pulled out of the pool for another request. Otherwise, I'm having to set max idle connections to 0.