go1.8: support for additional features

[kardianos]

Many new important features will land in go1.8. Please see document: https://docs.google.com/document/d/1F778e7ZSNiSmbju3jsEWzShcb8lIO4kDyfKDNm4PNd8/edit?usp=sharing for the full list.

Because these features are new we want drivers to implement them before they are released. Ideally they should be implemented by 2016-12-12 to allow for any driver provided feedback from implementation if necessary.

[alexbrainman]

I am not sure I will have time to investigate this properly. I do not need any of these new features myself. And I am out of touch with current database/sql changes. If you want to create some new tests in github.com/alexbrainman/odbc/mssql_test.go, that should pass, but do not, I will try and make them work. Even if tests are rough - as long as they clearly demonstrate problems that can be solved by my driver.

Thank you.

Alex

[mmargerumImpactrx]

The ability to navigate multiple result sets from a SQL Server stored procedure is the only thing Go is really lacking for me.

[alexbrainman]

The ability to navigate multiple result sets from a SQL Server stored procedure is the only thing Go is really lacking for me.

@mmargerumImpactrx do you know how your code (to use that functionality) would look like? If you do, please, create new issue with small broken example that I could fix. That would be helpful.

Thank you.

Alex

[esap]

This is my test code below:

sql := "select * from table1 select * from table2 select * from table3 "
rows,_ := db.Query(sql)
for rows.Next() {
   // do something
}
for rows.NextResultSet() {  // this is not implemented
  for rows.Next() {
    // do something else
  }
}

Multiple result sets is one of the features in go 1.8 which is useful. We hope it can be implemented in this driver.

[alexbrainman]

@kardianos how should I implement HasNextResultSet ? I can only see SQLMoreResults, but SQLMoreResults also advancing current ODBC statement to the next "result set". Is that how you expected HasNextResultSet to be implemented?

Thank you.

Alex

[kardianos]

@alexbrainman hardcode HasNextResultSet() bool {return true}. The only effect is that it won't auto-close the row set when the last result set is gotten to, which really shouldn't matter anyway.

[alexbrainman]

hardcode HasNextResultSet() bool {return true}

Thank you. That works for me. I do not understand how database/sql works, but maybe this should be suggested in the doc?

I hit another snag. I made this change:

diff --git a/mssql_test.go b/mssql_test.go
index f6ae527..45e757a 100644
--- a/mssql_test.go
+++ b/mssql_test.go
@@ -709,7 +709,10 @@ func TestMSSQLTypes(t *testing.T) {
    if is2008OrLater(db) {
        tests = append(tests, typeMSSQL2008Tests...)
    }
+   now := time.Now()
    for _, r := range tests {
+       fmt.Printf("duratiion=%v\n", time.Since(now))
+       now = time.Now()
        rows, err := db.Query(r.query)
        if err != nil {
            t.Errorf("db.Query(%q) failed: %v", r.query, err)
diff --git a/rows.go b/rows.go
index bcc80ef..a7c04a0 100644
--- a/rows.go
+++ b/rows.go
@@ -44,3 +44,11 @@ func (r *Rows) Next(dest []driver.Value) error {
 func (r *Rows) Close() error {
    return r.os.closeByRows()
 }
+
+func (r *Rows) HasNextResultSet() bool {
+   return true
+}
+
+func (r *Rows) NextResultSet() error {
+   return io.EOF
+}

and suddenly TestMSSQLTypes is really slow. I also noticed that each test loop takes about second to run:

...
duratiion=1.0048828s
duratiion=1.0068359s
duratiion=1.0048828s
duratiion=1.0048828s
duratiion=1.0048829s
duratiion=1.0039063s
duratiion=1.0048828s
duratiion=1.0058594s
duratiion=1.0029297s
duratiion=1.0058594s
duratiion=1.0048828s
duratiion=1.0058594s
...

So I grepped for "Second" in database/sql, and I find "const minInterval = time.Second". Do you think it is related? What should I do to debug that? Thank you. (Sorry for pumping you for information, otherwise I won't have time to advance this.)

Alex

[alexbrainman]

@kardianos another problem. I pushed my change into https://github.com/alexbrainman/odbc/tree/implement_NextResultSet , and now one of my tests fail

=== RUN   TestMSSQLRawBytes
--- FAIL: TestMSSQLRawBytes (0.02s)
        mssql_test.go:137: unexpected StmtCount: should=0, is=1
FAIL

So I am leaking connection handles now. What should I do about that? Thank you.

Alex

[kardianos]

Are all tests closing rows or canceling the query context?

On Wed, Apr 12, 2017, 23:50 Alex Brainman notifications@github.com wrote:

@kardianos https://github.com/kardianos another problem. I pushed my change into https://github.com/alexbrainman/odbc/tree/implement_NextResultSet , and now one of my tests fail

=== RUN TestMSSQLRawBytes --- FAIL: TestMSSQLRawBytes (0.02s) mssql_test.go:137: unexpected StmtCount: should=0, is=1 FAIL

So I am leaking connection handles now. What should I do about that? Thank you.

Alex

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrainman/odbc/issues/81#issuecomment-293806146, or mute the thread https://github.com/notifications/unsubscribe-auth/AAuFsZwQ6gi2efiVoBVlJuZhoXJOFadIks5rvcXRgaJpZM4KmUJT .

[kardianos]

If you want I could take a look at it. In a public branch?

[alexbrainman]

Are all tests closing rows or canceling the query context?

I wasn't closing rows in the test. I fixed it now 632bcac255d9e26a89bff5eff914d449e431f7b5 Thank you.

But, please, have a look at why TestMSSQLTypes is slow. I have created implement_NextResultSet branch in github.com/alexbrainman/odbc. You will, obviously, need MS SQL Server. And I use

go test -msdriver="SQL Server Native Client 10.0" -mssrv=server -msdb=db -msuser=user -mspass=password -run=TestMSSQLTypes github.com/alexbrainman/odbc

to run the test. Also note "SQL Server Native Client 10.0", because it works fine with "sql server" - these are just different MS SQL drivers.

Thank you

Alex

[wegged]

hey noticed that there hasnt been any movement on this in a while we use your library as well and it seems like most of the work is done for this what would it take to get this over the finish line,

[wegged]

I tried this branch and there actually seems to be an issue if the result sets have different number of columns. The fix was straightforward.

 func (r *Rows) NextResultSet() error {
    ret := api.SQLMoreResults(r.os.h)
    if ret == api.SQL_NO_DATA {
        return io.EOF
    }
    if IsError(ret) {
        return NewError("SQLMoreResults", r.os.h)
    }
    err := r.os.BindColumns()
    if err != nil {
        return err
    }
    return nil
}

[alexbrainman]

hey noticed that there hasnt been any movement on this in a while

Life gets in a way of this development. Sorry.

I tried this branch and there actually seems to be an issue if the result sets have different number of columns.

You are. probably, correct that there is a bug here. We would have to adjust the test in that change to prove there is a bug there. And to make sure your additional code fixes that bug.

Also do not forget about my comment from https://github.com/alexbrainman/odbc/issues/81#issuecomment-294107538

But, please, have a look at why TestMSSQLTypes is slow.

We have to verify that new code does not slow down things. Mind you code in database/sql package changes in the last year, so things might be very different now.

If you want to push this change along, I am all for it. I will review your code. Alternatively, you would have to wait until I find some free time.

Alex

[wegged]

So the reason why the change caused the slow down is because HasNextResultSet will always return true. This causes a change in behavior in this test. Because previously rows.Close() would be called automatically on every single rows.Next() b/c it proactively tries to close if there are no further rows. now it relies on the defer rows.Close() which will not get executed until the function terminates which causes it to create a new connection with each test.

[wegged]

@alexbrainman , I was able to confirm the above behavior by rewriting the test code to use a separate func per test run. Now the time to run the test is consistent. I have not found a solution that allows us to see if the statement has more result sets w/o moving to the next result set. If the change in behavior is acceptable to you I will make a PR with the tests and the updated code otherwise I will keep what we have forked. Please let me know asap as we need to decide if we will continue using this repo or move to our own for the project we are working on.

[wegged]

another option that may be worth considering is relying on the documentation for the NextResultSet interface and assume that HasNextResultSet will only be called when there is no more rows in a result set.

That we can change it so that HasNextResultSet actually moves to the next result and return true/false. If successful we would need some method of determining if we can skip calling SQLMoreResults again. Maybe this can be as simple as a counter that we increment and decrements.

[alexbrainman]

previously rows.Close() would be called automatically on every single rows.Next() b/c it proactively tries to close if there are no further rows. now it relies on the defer rows.Close() which will not get executed until the function terminates which causes it to create a new connection with each test.

I can see it now too. I changed deferred Close to immediate Close (in the loop), and now TestMSSQLTypes is quick again. Thank you very much.

But I still have 2 problems.

First your change https://github.com/alexbrainman/odbc/issues/81#issuecomment-449462819 has call to BindColumns unlike my version of that change. Why do you need it? Do you have a test that demonstrate the need for BindColumns?

And second, #127 is related to this issue. But, if I run odbc_test.txt (attached to the issue), it still fails even after I made changes similar to https://github.com/alexbrainman/odbc/issues/81#issuecomment-449462819. So I need more time to investigate what else is required here, before deciding on final change.

I will happily consider something that solves my last two problems.

In the meantime I pushed my latest changes at https://github.com/alexbrainman/odbc/tree/implement_NextResultSet

Thank you.

Alex

[wegged]

for a test that demonstrates the need try this sql you should get an error when you scan the row in the second result set because there is no second column. Ill see if I can figure out whats going on in #127

select 1
select 1, 2