Add support for the driver.Connector interface.

[flimzy]

Fixes #40

[flimzy]

Not sure if New is the best function name for this. I'm open to alternative suggestions.

[l3pp4rd]

Hi it is very important what dsn is used for txdb return d.Open("connector")
it serves as an identifier for transaction, that way, it can open separate transactions and run tests in parallel.
If the dsn is hardcoded, it means no concurrent support with Connector.

Also, we cannot just increment the number there, since in some cases it is rather important to reuse same connection. since in applications it sometimes uses same database at the same time but with different connections.

So it must be possible to specify the dsn in a clean way and reuse or create a separate transaction based on it.
But given the way it is handled now, it may not be that easy to come up with good implementation

[flimzy]

Hi it is very important what dsn is used for txdb return d.Open("connector")
it serves as an identifier for transaction, that way, it can open separate transactions and run tests in parallel.
If the dsn is hardcoded, it means no concurrent support with Connector.

Also, we cannot just increment the number there, since in some cases it is rather important to reuse same connection. since in applications it sometimes uses same database at the same time but with different connections.

So it must be possible to specify the dsn in a clean way and reuse or create a separate transaction based on it.
But given the way it is handled now, it may not be that easy to come up with good implementation

Based on my reading of the code, that dsn is only used as a map in the *txDriver object, and not globally.

So the way I envision enabling separate transactions here is by making separate calls to txdb.New(). I.e:

txn1, err := sql.OpenDB(txdb.New("mysql", "root@/txdb_test"))
/* ... */
txn2, err := sql.OpenDB(txdb.New("mysql", "root@/txdb_test"))

This should be equivalent to the old way:

txdb.Register("txdb", "mysql", "root@/txdb_test")
/* ... */
txn1, err := sql.Open("txdb", "unique_string_1")
/* ... */
txn2, err := sql.Open("txdb", "unique_string_2")

Or maybe there's some implication of the dsn I'm not seeing immediately?

[l3pp4rd]

txdb.Register in go, creates txdriver instance only once. Every time db.open is called with dsn, the driver looks into the dsn to transaction connection map based on that dsn it either opens a new transaction or reuses the transaction which is already open (in this case every action to that transaction is protected with mutex)
With connector implementation, it would do only one way, exactly as you explain. Create the separate txdriver instance every time with that dsn to connection map with single and only dsn in it. If your tested service lets say opens db in two or more places, it would start two separate transactions and would not be usable.

[flimzy]

txdb.Register in go, creates txdriver instance only once. Every time db.open is called with dsn, the driver looks into the dsn to transaction connection map based on that dsn it either opens a new transaction or reuses the transaction which is already open (in this case every action to that transaction is protected with mutex)
With connector implementation, it would do only one way, exactly as you explain. Create the separate txdriver instance every time with that dsn to connection map with single and only dsn in it. If your tested service lets say opens db in two or more places, it would start two separate transactions and would not be usable.

Are you saying there's a difference in behavior between these two approaches? I don't see any difference.

It's already possible, using the Register method, to create multiple simultaneous transactions, by providing different dsns. This is no different than creating multiple Connector instances with New. (It's also no different than registering multiple txdb drivers to the same db, which is also possible today).

The only difference I see is where the connection disambiguation occurs. With the Register approach, the conns map is used in conjunction with the dsn value passed to Open. With the Connector approach, it's handled by the instantiation of the Connector instance.

Logically, I believe these are identical.

The mutexes I see only protect the members of txDriver and conn respectively. I don't see any mutexes protecting transactions, per se. Maybe I'm misunderstanding the point you were making about mutexes, though.

[lopezator]

@flimzy would you be open to recover this? It seems that Connector is the new way to go and that the drivers should start using that pattern instead.

https://pkg.go.dev/database/sql/driver

[flimzy]

@l3pp4rd Any objection to me updating this PR to get it merged?

[lopezator]

Any objection to me updating this PR to get it merged?

That was fast! Truly appreciated.

[flimzy]

@lopezator This project has seemingly gone without a lot of attention for quite a while. I'm sure this PR just got lost when attention was focused on other tasks at DATA-DOG. Hopefully we can get this ball rolling again.

[Yiling-J]

@flimzy Looks good, would be better if adding some unit tests, or refactoring current tests to also init db using sql.OpenDB method

[flimzy]

@Yiling-J Good suggestion. I've added a very simple test to exercise just the new code. WDYT?

[lopezator]

Should we take the opportunity to implement the DriverContext interface as well? But maybe that's a separate PR.

[flimzy]

Should we take the opportunity to implement the DriverContext interface as well? But maybe that's a separate PR.

I agree on both counts. It sounds like something we should definitely support, and it should be a separate PR.