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

contrib/database/sql: Support go-mssqldb driver #1163

Closed
wants to merge 10 commits into from

Conversation

ajgajg1134
Copy link
Contributor

@ajgajg1134 ajgajg1134 commented Feb 8, 2022

The go-mssqldb driver doesn't implement some of the optional sql.Driver interfaces the code previously expected. This change adds checks for other interfaces that can be used.

TODO: I've added an integration test for sqlserver to verify these changes worked locally, but am unsure how to add it to the circleci pipeline. Got it working 😎

Fixes #1043

The go-mssqldb driver doesn't implement some of the optional sql.Driver interfaces the code previously expected. This change adds checks for other interfaces that can be used.

Fixes #1043
@ajgajg1134 ajgajg1134 added this to the 1.37.0 milestone Feb 8, 2022
@ajgajg1134 ajgajg1134 marked this pull request as ready for review February 8, 2022 21:37
@ajgajg1134 ajgajg1134 requested a review from a team as a code owner February 8, 2022 21:37
@ajgajg1134 ajgajg1134 requested review from a team and knusbaum February 8, 2022 21:37
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find. This is complicated code.

So, this is really spooky to me...
We wrap a driver.Conn but implement optional driver package interfaces that the underlying driver.Conn may not...

This means we need to emulate that behavior - key word being "emulate". The execution flow will take a different path through the database/sql code than an untraced connection would, meaning it's on us to make sure our code does what the database/sql code does when we need to emulate that behavior.

For instance, this bug was initiated here, in queryDC:
https://github.com/golang/go/blob/9cec77ac11b012283e654b423cf85cf9976bedd9/src/database/sql/sql.go#L1744-L1782

The untraced driver implemented neither driver.QueryerContext nor driver.Queryer, and so the code skipped over the if ok { branch:
https://github.com/golang/go/blob/9cec77ac11b012283e654b423cf85cf9976bedd9/src/database/sql/sql.go#L1745-L1750
instead continuing and calling ctxDriverPrepare:
https://github.com/golang/go/blob/9cec77ac11b012283e654b423cf85cf9976bedd9/src/database/sql/sql.go#L1778-L1786

When we wrapped the driver.Conn, our wrapper implements both driver.QueryerContext and driver.Queryer, meaning ctxDriverQuery gets called:
https://github.com/golang/go/blob/9cec77ac11b012283e654b423cf85cf9976bedd9/src/database/sql/sql.go#L1755-L1759

I'm concerned about trying to fix this via mimicking database/sql behavior. That code is complicated, and proving to ourselves that this is equivalent for all relevant cases is going to be very difficult - leading to bugs like this one. Even if we manage to make it work, it still means that traced code may follow a totally different execution path than non-traced code - never ideal.

Instead I'm wondering if we can implement something similar to our http integrations ResponseWriter
See:

Meaning we would use a driver.Conn that would only implement the interfaces the underlying type implemented. It may be a little gross, but it might save us from things like this.

.circleci/config.yml Outdated Show resolved Hide resolved
@ajgajg1134
Copy link
Contributor Author

this work was merged in PR #1167

@ajgajg1134 ajgajg1134 closed this Feb 28, 2022
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.

contrib/database/sql: error for named parameters when using mssql driver for sqltrace
2 participants