Add support for drivers with ConnBeginTx interface #214
Conversation
| if !ok { | ||
| return nil, driver.ErrSkip | ||
| } |
There was a problem hiding this comment.
Do we really need an extra driver config ?
Will this condition not take care of any driver not implementing `ConnBeginTx' ?
There was a problem hiding this comment.
I am not sure, it seems from this code that the go sql library will handle it differently if the interface is implemented or not
https://github.com/golang/go/blob/master/src/database/sql/ctxutil.go#L97-L135
As you can see from that code if the interface is not implemented there is a fallback that eventually returns
errors.New("sql: driver does not support read-only transactions")Thats how I originally found this issue
There was a problem hiding this comment.
What i meant is do we need to create a new struct sqlCommenterConnWithTx ?
Can we just not implement BeginTx for sqlCommenterConn ?
func (s *sqlCommenterConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) {...}
There was a problem hiding this comment.
I understand that :).
It's mainly your call.
Doing so makes the code simpler but it may change the expected behavior for drivers that don't implement BeginTx as the code in go library has a special handling for drivers that don't implement that interface.
|
Thanks @kostyay for this PR ! |
|
Did you decide which way you want to implement this? |
Yes, this way should be fine for now. |
|
@mickeyreiss Changes looks good. We may need to add some unit tests for the new changes to avoid regression. |
Some drivers implement the
ConnBeginTxinterface, without it its not possible to specify Isolation mode for queries in Postgres.So I've implemented it.
I wanted to keep the behavior the same for drivers that don't implement it (so database/sql will fail with the
driver does not support non-default isolation levelerror)If you like this implementation let me know and I will add a test.