search

alexbrainman / odbc

odbc driver written in go
BSD 3-Clause "New" or "Revised" License
348 stars 139 forks source link

Add 1.8 Context Method Support #151

Closed ninthclowd closed 1 year ago

ninthclowd commented 3 years ago

Hello, first of all thanks so much for building this library; your work has been a great help :)

TL;DR

Statements executed via context will now return immediately if the context expires, the connection will be invalidated so the driver doesn't reuse it, and then in the background:

In addition this adds the ability to change isolation levels of the transaction when calling BeginTx.

Some of the thread safety has been simplified and/or removed, as go1.10 seems to make sure that statements and connections aren't used concurrently providing the SessionResetter is working correctly.

I also Vendor'd the module

Interface Changes

The driver now implements the following interfaces:

The connection now implements the following interfaces:

The statement now implements:

Package restructure

The package / file structure has been refactored to resemble some other sql.Driver packages out there like pq. Some of the restructure is as follows:

If this release is merged, could we get a v1.0.0 tag on the current release, and a new vesion for this release so users can opt out if they desire to?

Again thanks for all your work on this.

-John

ninthclowd commented 3 years ago

I'm going to close this until I can find time to test more thoroughly :)

ninthclowd commented 3 years ago

Ok I was able to get all of the tests passing today on both windows (386) and linux. I did have to add thread safety to the read operations on driver.Stats to allow the race test to pass (write was thread safe, just not read).

On windows tested using the following drivers:

alexbrainman commented 3 years ago

Hello John,

Thank you for making the effort to create this PR.

Unfortunately the PR is too big for me to review. I have full time job and I also try to contribute to some open source projects. So I have to decide where my time goes.

Unfortunately I don't see how I can spend time reviewing PR with 20 commits and 144 files changed.

I briefly looked at your changes. And what I see is nice.

But I have been using this package for many years myself. Personally I don't find any need to change anything here. I suspect many package users are in the same situation.

And I also think that there are better alternatives to this package especially on non-Windows. So some users should be using different packages that are available,

Another problem I have is that I cannot test your changes.

I had Linux computer with docker installed where I could run MSSQL server on it. But that computer is dead now. So I have to find some Linux PC and have docker running on it, and make sure my setup works.

I also had Linux computer with unixODBC and FreeTDS installed on it, so I can run this package tests on Linux. That computer is dead too. So I would have to install all driver bits on Linux again.

I suppose I have no choice but to install all this to test your changes. But that will take time. And I would have to do it even before reviewing your code.

Anyway, if we agree to proceed with this work, I ask you to split this PR into smaller PRs. Preferably 1 commit (or a few) that completes preferably single task. So we can review the commit, run tests on it, submit it and start using it.

I don't want to break existing users (including myself) who don't care about these changes.

Please try not to add any new dependencies to this code. But let's discuss this when particular code is reviewed.

I also don't care if existing API of this package changes or not. This package was designed long time ago - when Go had no package versions support. I never even considered users who will use this package in a different way from just silently importing github.com/alexbrainman/odbc. I still don't care about those users. I don't plan to provide any backward compatibility for the package API. I will try not to change API, but if we need to change the API, it is fine with me.

Please let me know what you want to do.

Thank you.

Alex

ninthclowd commented 3 years ago

Alex, thanks for your reply.

Unfortunately I don't see how I can spend time reviewing PR with 20 commits and 144 files changed.

Ya... I was kind of worried about that. I think most of that is the vendor'ing though. I'll try to think on how we could split it up and make it easier to digest.

But I have been using this package for many years myself. Personally I don't find any need to change anything here. I suspect many package users are in the same situation.

This driver uses the deprecated driver.Execer and driver.Queryer interfaces. As a result, modern go developers who use this library will get the impression they can use ExecContext and QueryContext, yet when they do, the methods don't return when the context is canceled.

We fell victim to this, and originally ended up working around the issue in our web application. However, the queries would still be running in the background, which could lead to denial of service. This is why I undertook this effort, and we are currently using the context enable odbc driver version from my fork on our web servers with no issues. I will definitely report if we find an issue.

Another problem I have is that I cannot test your changes.

That was a challenge for me as well. FWIW, I did ensure that all of your tests pass on both windows and linux with the new version, but obviously someone else should validate the changes.

While we are content using the fork from my repo, I imaging there are other users who are looking to use this on a web server. Issue #137 seems to suggest this. Personally I would like to get the value to them as well, but this is your repository, so you tell me what you want to do :)

Again, thanks for your work on this library. While you may think there are better options available, I haven't found them. I am very grateful for your contribution.

alexbrainman commented 3 years ago

@ninthclowd I will wait for you to make changes as I requested.

Let me know, if you expect something different from me.

Thank you.

Alex

ninthclowd commented 1 year ago

I'm going to close this request as we are now maintaining our own fork with the context cancellation functionality we need and I feel it would be a pretty large effort to integrate this with the primary trunk based on your previous comments. If I ever find the time to re-work this I will submit a different PR. Thanks

alexbrainman commented 1 year ago

I'm going to close this request as we are now maintaining our own fork with the context cancellation functionality we need and I feel it would be a pretty large effort to integrate this with the primary trunk based on your previous comments. If I ever find the time to re-work this I will submit a different PR.

SGTM. Thanks for letting me know.

Alex