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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PuppyKhan
Copy link

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

ziutek commented Apr 7, 2015

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

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

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

@lucalooz
Copy link

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

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants