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

Add support for drivers with ConnBeginTx interface #214

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kostyay
Copy link
Contributor

@kostyay kostyay commented Dec 12, 2022

Some drivers implement the ConnBeginTx interface, 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 level error)
If you like this implementation let me know and I will add a test.

Comment on lines +107 to +109
if !ok {
return nil, driver.ErrSkip
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need an extra driver config ?

Will this condition not take care of any driver not implementing `ConnBeginTx' ?

Copy link
Contributor Author

@kostyay kostyay Dec 14, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@sjs994 sjs994 Dec 15, 2022

Choose a reason for hiding this comment

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

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) {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@sjs994
Copy link
Collaborator

sjs994 commented Dec 14, 2022

Thanks @kostyay for this PR !

@kostyay
Copy link
Contributor Author

kostyay commented Dec 25, 2022

Did you decide which way you want to implement this?
It would be great to have this merged, I rather not work with a sidebranch :)

@sjs994
Copy link
Collaborator

sjs994 commented Dec 26, 2022

Did you decide which way you want to implement this? It would be great to have this merged, I rather not work with a sidebranch :)

Yes, this way should be fine for now.
Can you add unit tests and then we can review and merge this PR.

@mickeyreiss
Copy link

@sjs994 @kostyay Any blockers to rebasing and merging this?

@sjs994
Copy link
Collaborator

sjs994 commented May 9, 2024

@mickeyreiss Changes looks good. We may need to add some unit tests for the new changes to avoid regression.

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.

3 participants