Implement driver.ConnBeginTx

[m12i]

This change implements driver.ConnBeginTx interface so that sqlhooks can be used when a non default isolation level is used.

---

This change is 

[coveralls]

Coverage decreased (-1.7%) to 72.889% when pulling 297e933 on coordcity:master into 1932c8d on gchaincl:master.

[coveralls]

Coverage decreased (-1.7%) to 72.889% when pulling 297e933 on coordcity:master into 1932c8d on gchaincl:master.

[coveralls]

Coverage decreased (-1.7%) to 72.889% when pulling 297e933 on coordcity:master into 1932c8d on gchaincl:master.

[coveralls]

Coverage decreased (-1.7%) to 72.889% when pulling 297e933 on coordcity:master into 1932c8d on gchaincl:master.

[coveralls]

Coverage increased (+0.1%) to 74.667% when pulling 21d7370 on coordcity:master into 1932c8d on gchaincl:master.

[qustavo]

Is there any way to test this?

[m12i]

Sure. Added a test

Reviewable status: 0 of 2 files reviewed, all discussions resolved

[m12i]

@gchaincl Just bringing this PR to your attention again

[qustavo]

Thanks for addressing this and sorry for not getting back, I'll review it this week.

…

On Mon, Mar 25, 2019, 15:06 Mahmood Rahmani ***@***.***> wrote: Just bringing this PR to your attention again — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABOoeWIpuB-Z-H5pHZ6ZANmVyiRASAmQks5vaNf1gaJpZM4btXdd> .

[qustavo]

Did you notice any impact on the benchmarks? Other than that LGTM!

[m12i]

The performance seem unchanged (as expected):

go test -bench=. -benchmem on master:

BenchmarkSQLite3/Without_Hooks-4         	  200000	      7815 ns/op	    1072 B/op	      18 allocs/op
BenchmarkSQLite3/With_Hooks-4            	  200000	      8098 ns/op	    1072 B/op	      18 allocs/op

go test -bench=. -benchmem on this PR:

BenchmarkSQLite3/Without_Hooks-4         	  200000	      7972 ns/op	    1072 B/op	      18 allocs/op
BenchmarkSQLite3/With_Hooks-4            	  200000	      8094 ns/op	    1072 B/op	      18 allocs/op

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @keegancsmith)

[keegancsmith]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @m12i)

---

sqlhooks.go, line 167 at r2 (raw file):

		return ciCtx.BeginTx(ctx, opts)
	}
	return nil, errors.New("sqlhooks: driver does not support non-default isolation level")

Are there any drivers that don't implement BeginTx? Because I assume this would regress sqlhooks wrapping those drivers.

[m12i]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @keegancsmith)

---

sqlhooks.go, line 167 at r2 (raw file):

Previously, keegancsmith (Keegan Carruthers-Smith) wrote…

Are there any drivers that don't implement BeginTx? Because I assume this would regress sqlhooks wrapping those drivers.

Good point, I checked all major sql drivers and they do implement diver.ConnBeginTx.

Even if a driver does not implement diver.ConnBeginTx, this PR wouldn't change any outcome as calling their BeginTx method would fail anyway, with or without being wrapped by sqlhooks.
I changed the error message to be more accurate.

[m12i]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @keegancsmith)

---

sqlhooks.go, line 167 at r2 (raw file):

Previously, m12i (Mahmood Rahmani) wrote…

Good point, I checked all major sql drivers and they do implement diver.ConnBeginTx.

Even if a driver does not implement diver.ConnBeginTx, this PR wouldn't change any outcome as calling their BeginTx method would fail anyway, with or without being wrapped by sqlhooks.
I changed the error message to be more accurate.

Done.

[keegancsmith]

aah I see now from the godoc of BeginTx:

If a non-default isolation level is used that the driver doesn't support, an error will be returned.

Ok so this retains the behaviour. I would actually duplicate what sql/db returns in the case the interface is not implemented: https://sourcegraph.com/github.com/golang/go@go1.12/-/blob/src/database/sql/ctxutil.go#L110-120

[m12i]

That's actually what I thought in the beginning hence returning the previous error message. But if you look at the code in func ctxDriverBegin(...) the sqlhooks' BeginTx() is called in the first if statement (line 106). while errors of line 113 or 119 are for drivers who don't support BeginTx() but still are trying to have non-default opts.

So there are two options for sqlhooks:
1- Say sqlhooks supports only drivers that support transaction (i.e. implement BeginTx())
2- duplicate the whole logic of func ctxDriverBegin() in sqlhooks' BeginTx()

option 2 does not look like a good idea, as every time func ctxDriverBegin() changes it may make sqlhooks duplicated code inconsistent.
option 1 seems okey (not ideal). The reason it's okey is as follows:
There are two types of drivers: the ones who implement BeginTx() and the ones who don't. ctxutil.go behaves differently depending on whether or not the driver supports BeginTx(). For sqlhooks as a wrapper I can't think of a way to support both types of drivers. because sqlhooks cannot make itself a supporter and non-supporter of BeginTx() at the same time.

I hope what I said is clear. Please let me know what you think.

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @keegancsmith)

[m12i]

BTW, in my first response to you, I was wrong about drivers' who don't implement BeginTx(). They only fail if they start a transaction with a non-default opt.

Here is one more reason for sqlhooks to implement BeginTx(). Majority of drivers implement BeginTx(). if you wrap them with sqlhooks they no longer support BeginTx() which to me sounds like downgrading those drivers.
A minority (perhaps less popular) drivers don't implement BeginTx(). Since there is no way to support both, I think sqlhooks should stick with the majority (and most popular) drivers. Does that make sense?

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @keegancsmith)

[keegancsmith]

Since there is no way to support both, I think sqlhooks should stick with the majority (and most popular) drivers. Does that make sense?

There is a way, you do type checks against the driver to find out, if it does you return a driver which does support BeginCtx. We do that in other parts of sqlhooks. However, given what you said I agree. Rather make sqlhooks just support the majority. Not sure what @gchaincl thinks.

[m12i]

Where would you check against the driver? if it's inside theBeginTx it's too late. I don't see how that can be done. Could you please elaborate?

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @keegancsmith)

[keegancsmith]

See the Open function.

[m12i]

oh I see. I did start checking it in Open() but it would become a combinatorial explosion. Would that be like 5 more ifs to count for the combination of BeginCtxer and all existing ifs ?

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @keegancsmith)

[qustavo]

I'm not sure about what's the best choice here, although dropping support for the drivers that don't implement BeginTx sounds reasonable, it's not ideal since we (or at least I) don't know for sure what the affected drivers are. I like the idea of duplicating what https://sourcegraph.com/github.com/golang/go@go1.12/-/blob/src/database/sql/ctxutil.go#L110-120 is doing, even if the code might change, the impact shouldn't huge.

[qustavo]

any progress on this @m12i ?