Avoid keeping output parameters in the connection

[tc-hib]

This is to fix #654

My knowledge on this driver is very limited, so I hope someone will be able to carefully review this.

What this does is:

• Implement NamedValueChecker in Stmt instead of Conn.
• Move output parameters (outs and returnStatus) from the Conn to the Stmt.
• When clearing output parameters, clear return status as well.
• Don't pass output parameters anymore to Begin/Commit/Rollback (I hope it's OK)

[codecov[bot]]

Codecov Report

Merging #656 (26271a7) into master (d9abbec) will decrease coverage by 0.09%. The diff coverage is 90.62%.

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
- Coverage   72.33%   72.24%   -0.10%     
==========================================
  Files          24       24              
  Lines        5469     5472       +3     
==========================================
- Hits         3956     3953       -3     
- Misses       1289     1293       +4     
- Partials      224      226       +2     

Impacted Files	Coverage Δ
mssql.go	87.03% <81.25%> (-0.40%)	:arrow_down:
bulkcopy.go	57.33% <100.00%> (ø)
mssql_go19.go	95.65% <100.00%> (ø)
tds.go	66.66% <100.00%> (-0.56%)	:arrow_down:
token.go	61.11% <100.00%> (+0.18%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d9abbec...26271a7. Read the comment docs.

[shogo82148]

I think that output parameters should be managed by the Conn.

Firstly, moving output parameters from the Conn to the Stmt makes hard to optimize by implementing driver.QueryerContext. I don't know is there some plan to implement it, but the more choices we have, the better.

Secondly, your example in https://github.com/denisenkom/go-mssqldb/issues/654 closes Stmts for each QueryContext call. But users can re-use Stmts, e.g.:

func main() {
    db, err := sql.Open("sqlserver", "sqlserver://sa:sa@localhost/sqlexpress?database=master")
    if err != nil {
        log.Fatalln(err)
    }
    defer db.Close()

    stmt, _ := db.PrepareContext(context.TODO(), "exec sp_recompile 'bad'")
    defer stmt.Close()

    var rs mssql.ReturnStatus
    r, _ := stmt.QueryContext(context.TODO(), &rs)
    if r != nil {
        r.Close()
    }
    rs = 0
    r, _ = stmt.QueryContext(context.TODO()) // don't pass &rs
    if r != nil {
        r.Close()
    }
    if rs != 0 {
        log.Fatalln("rs has been overwritten by the second query, the connection was the same")
    }
    log.Println("rs == 0, the second query ran on another connection, or the bug is fixed")
}

I haven't checked, but it may have just happened to work well in your example. I don't think it's a fundamental solution.

[tc-hib]

Hi, thanks for having a look at it!

I think that output parameters should be managed by the Conn.

Firstly, moving output parameters from the Conn to the Stmt makes hard to optimize by implementing driver.QueryerContext. I don't know is there some plan to implement it, but the more choices we have, the better.

Alright, I wrongly assumed there always was an Stmt, either prepared or implicit. I'm learning little by little what sql drivers are supposed to do :)

Do you have an idea of how we could safely handle "query scope" as opposed to connection/statement scope ?

I think we should at least clear returnStatus where we already clear outs, and probably clear them when we get an error before startReading is called, to avoid side effects.

[tc-hib]

Maybe something like this? (adding a defer in every entry point that takes query argument)

[shogo82148]

I commented on #654 for tracking the issue.