Skip to content

Add support for driver.ConnBeginTx #33

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 21, 2021
Merged

Add support for driver.ConnBeginTx #33

merged 2 commits into from
Jan 21, 2021

Conversation

asdine
Copy link
Collaborator

@asdine asdine commented Jan 18, 2021 •
edited by qustavo
Loading

This PR adds support for drivers implementing the driver.ConnBeginTx interface.

Fixes #20
Closes #24

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 72.202% when pulling b4e3892 on asdine:support-connbegintx into e01e589 on gchaincl:master.

@coveralls
Copy link

coveralls commented Jan 18, 2021 •
edited
Loading

Coverage Status

Coverage decreased (-2.2%) to 72.924% when pulling 4803583 on asdine:support-connbegintx into e01e589 on gchaincl:master.

@qustavo
Copy link
Owner

qustavo commented Jan 18, 2021

Thank @asdine for doing this!
Any change to add a test for this?

@asdine
Copy link
Collaborator Author

asdine commented Jan 19, 2021

Hey @gchaincl ! I added a test in here https://github.com/gchaincl/sqlhooks/blob/b4e3892e2d1633b251602e96e1be9f83c77a82b3/sqlhooks_interface_test.go#L66-L71, do you have something else in mind?

@asdine asdine mentioned this pull request Jan 19, 2021
Merged
@qustavo
Copy link
Owner

qustavo commented Jan 19, 2021

Hey @gchaincl ! I added a test in here

https://github.com/gchaincl/sqlhooks/blob/b4e3892e2d1633b251602e96e1be9f83c77a82b3/sqlhooks_interface_test.go#L66-L71
, do you have something else in mind?

I think you can add it here: https://github.com/gchaincl/sqlhooks/blob/b4e3892e2d1633b251602e96e1be9f83c77a82b3/sqlhooks_interface_test.go#L15

@asdine
Copy link
Collaborator Author

asdine commented Jan 19, 2021

I realized I didn't add it to that list and my solution doesn't work 🤦🏼‍♂️
After a few tries and reading #24, I think the best solution here is to add BeginTx to the base Conn type, dropping support for drivers not supporting driver.ConnBeginTx.

I have analyzed the drivers listed in https://github.com/golang/go/wiki/SQLDrivers:

53 drivers

Apache H2: https://github.com/jmrobles/h2go
Apache Ignite/GridGain: https://github.com/amsokol/ignite-go-client
Apache Impala: https://github.com/bippio/go-impala
Apache Avatica/Phoenix: https://github.com/apache/calcite-avatica-go
Amazon AWS Athena: https://github.com/uber/athenadriver
AWS Athena: https://github.com/segmentio/go-athena
Azure Cosmos DB: https://github.com/btnguyen2k/gocosmos
ClickHouse (uses native TCP interface): https://github.com/ClickHouse/clickhouse-go
ClickHouse (uses HTTP API): https://github.com/mailru/go-clickhouse
CockroachDB: Use any PostgreSQL driver
Couchbase N1QL: https://github.com/couchbase/go_n1ql
DB2 LUW and DB2/Z with DB2-Connect: https://bitbucket.org/phiggins/db2cli (Last updated 2015-08)
DB2 LUW (uses cgo): https://github.com/asifjalil/cli
DB2 LUW, z/OS, iSeries and Informix: https://github.com/ibmdb/go_ibm_db
Firebird SQL: https://github.com/nakagami/firebirdsql
Genji (pure go): https://github.com/genjidb/genji
Google Cloud BigQuery: https://github.com/solcates/go-sql-bigquery
Google Cloud Spanner: https://github.com/rakyll/go-sql-driver-spanner
MS ADODB: https://github.com/mattn/go-adodb
MS SQL Server (pure go): https://github.com/denisenkom/go-mssqldb
MS SQL Server (uses cgo): https://github.com/minus5/gofreetds
MySQL: https://github.com/go-sql-driver/mysql/ []
MySQL: https://github.com/siddontang/go-mysql/ [**] (also handles replication)
MySQL: https://github.com/ziutek/mymysql []
ODBC: https://bitbucket.org/miquella/mgodbc (Last updated 2016-02)
ODBC: https://github.com/alexbrainman/odbc
Oracle (uses cgo): https://github.com/mattn/go-oci8
Oracle (uses cgo): https://gopkg.in/rana/ora.v4
Oracle (uses cgo): https://github.com/godror/godror
Oracle (pure go): https://github.com/sijms/go-ora
QL: http://godoc.org/github.com/cznic/ql/driver
Postgres (pure Go): https://github.com/lib/pq []
Postgres (uses cgo): https://github.com/jbarham/gopgsqldriver
Postgres (pure Go): https://github.com/jackc/pgx [**]
Presto: https://github.com/prestodb/presto-go-client
SAP HANA (uses cgo): https://help.sap.com/viewer/0eec0d68141541d1b07893a39944924e/2.0.03/en-US/0ffbe86c9d9f44338441829c6bee15e6.html
SAP HANA (pure go): https://github.com/SAP/go-hdb
SAP ASE (uses cgo): https://github.com/SAP/go-ase - package cgo (pure go package planned)
Snowflake (pure Go): https://github.com/snowflakedb/gosnowflake
SQLite (uses cgo): https://github.com/mattn/go-sqlite3 []
SQLite (uses cgo): https://github.com/gwenn/gosqlite - Supports SQLite dynamic data typing
SQLite (uses cgo): https://github.com/mxk/go-sqlite
SQLite: (uses cgo): https://github.com/rsc/sqlite
SQLite: (pure go): https://modernc.org/sqlite
SQL over REST: https://github.com/adaptant-labs/go-sql-rest-driver
Sybase SQL Anywhere: https://github.com/a-palchikov/sqlago
Sybase ASE (pure go): https://github.com/thda/tds
TiDB: Use any MySQL driver
Vertica: https://github.com/vertica/vertica-sql-go
Vitess: https://godoc.org/vitess.io/vitess/go/vt/vitessdriver
YQL (Yahoo! Query Language): https://github.com/mattn/go-yql
Apache Hive: https://github.com/sql-machine-learning/gohive
MaxCompute: https://github.com/sql-machine-learning/gomaxcompute

12 don't support transactions at all (and thus are not compatible with SQLHooks)

Apache Ignite/GridGain: https://github.com/amsokol/ignite-go-client
Apache Impala: https://github.com/bippio/go-impala
Apache Avatica/Phoenix: https://github.com/apache/calcite-avatica-go
Amazon AWS Athena: https://github.com/uber/athenadriver
AWS Athena: https://github.com/segmentio/go-athena
Azure Cosmos DB: https://github.com/btnguyen2k/gocosmos
Couchbase N1QL: https://github.com/couchbase/go_n1ql
Presto: https://github.com/prestodb/presto-go-client
SQL over REST: https://github.com/adaptant-labs/go-sql-rest-driver
YQL (Yahoo! Query Language): https://github.com/mattn/go-yql
Apache Hive: https://github.com/sql-machine-learning/gohive
MaxCompute: https://github.com/sql-machine-learning/gomaxcompute

12 don't support BeginTx, 5 of them being archived or abandonned

DB2 LUW, z/OS, iSeries and Informix: https://github.com/ibmdb/go_ibm_db
Google Cloud BigQuery: https://github.com/solcates/go-sql-bigquery
MS SQL Server (uses cgo): https://github.com/minus5/gofreetds
MySQL: https://github.com/siddontang/go-mysql/
MySQL: https://github.com/ziutek/mymysql
ODBC: https://github.com/alexbrainman/odbc
Oracle (pure go): https://github.com/sijms/go-ora
QL: http://godoc.org/github.com/cznic/ql/driver (archived)
Postgres (uses cgo): https://github.com/jbarham/gopgsqldriver (last commit 9 years ago)
SQLite (uses cgo): https://github.com/mxk/go-sqlite (last commit 7 years ago)
SQLite: (uses cgo): https://github.com/rsc/sqlite (last commit 5 years ago)
Sybase SQL Anywhere: https://github.com/a-palchikov/sqlago (last commit 5 years ago)

What do you think?

@qustavo
Copy link
Owner

qustavo commented Jan 19, 2021

I wasn't aware that there were +50 drivers, that's crazy!
I'm really not sure how to do this, is there a way for sqlhooks to know if a driver doesn't support BeginTx?

@asdine
Copy link
Collaborator Author

asdine commented Jan 19, 2021

It is possible by running a simple type assertion. The problem is that because we have different types implementing different interfaces, we cannot write a wrapper around them: https://play.golang.org/p/9fD_7jKW6gt

One solution would be to add one additional struct for each implementation (one for *Execer, one for *ExecerQueryer, etc.), but that seems a bit overkill. Since very few active drivers don't support BeginTx (7), and since the type has been introduced since Go 1.8, I believe it would be easier to cut a new version with support for BeginTx and people using one of the 7 incompatible drivers could still use the current version of sqlhooks.

@qustavo
Copy link
Owner

qustavo commented Jan 19, 2021

That makes sense, which are the drivers that don't support BeginTx?

@asdine
Copy link
Collaborator Author

asdine commented Jan 19, 2021

which are the drivers that don't support BeginTx?

They are listed here in this comment, you can click on the arrows on this comment it's a dropdown (not the best UI choice sorry) #33 (comment)

@qustavo
Copy link
Owner

qustavo commented Jan 19, 2021

ah sorry, my bad.
happy to cut a new version and drop support for those drivers

@asdine asdine force-pushed the support-connbegintx branch from 95649df to e048d48 Compare January 20, 2021 09:43
@asdine asdine force-pushed the support-connbegintx branch from e048d48 to 4803583 Compare January 20, 2021 09:44
qustavo
qustavo approved these changes Jan 20, 2021
View reviewed changes
Copy link
Owner

@qustavo qustavo left a comment

Choose a reason for hiding this comment

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

Looks LGTM to me

@qustavo
Copy link
Owner

qustavo commented Jan 20, 2021

As this drops support for drivers that don't implement ConnBeginTx I think we should release v2.0.0

@asdine
Copy link
Collaborator Author

asdine commented Jan 21, 2021

@gchaincl Good idea, should I create a v2 directory or should we simply merge this and create a v2.0.0 release?

@qustavo
Copy link
Owner

qustavo commented Jan 21, 2021

I'd just merge it and release v2.0.0

@asdine asdine merged commit 9db12c7 into qustavo:master Jan 21, 2021
@asdine asdine deleted the support-connbegintx branch January 21, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qustavo qustavo qustavo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

can't use transactions with sqlhooks
3 participants
@asdine @coveralls @qustavo