elixir-ecto / db_connection

Database connection behaviour
http://hexdocs.pm/db_connection/DBConnection.html
306 stars 113 forks source link

New Release #92

Closed christianjgreen closed 6 years ago

christianjgreen commented 7 years ago

Is there an ETA on the latest release from the master branch? The new error handling for connections {:error, msg} is needed for a DB driver using this repo!

christianjgreen commented 7 years ago

@ankhers

michalmuskala commented 7 years ago

The new release is planned after postgrex and mariaex get updated, so we know the approach is correct.

christianjgreen commented 7 years ago

Is there a version you guys are looking for? Postgrex 12 days ago and Mariaex 3 days ago

ankhers commented 7 years ago

I believe the work for Postgrex is (mostly?) complete. I was asked to do the same for Mariaex, but my unfamiliarity with the database and codebase prevented me from getting it done.

I do not mind taking another crack at it. Unfortunately, even if I thought I got it working, my unfamiliarity with things would prevent me from knowing for sure.

michalmuskala commented 7 years ago

The work is being done in https://github.com/elixir-ecto/postgrex/pull/321 and https://github.com/xerions/mariaex/pull/196

fishcakez commented 7 years ago

@Arthien is the driver open source? If so can you link to an issue that we can track? It's possible we could look at a v1.1 branch whose parent commit is as far back as possible - before new features were introduced.

ankhers commented 7 years ago

@Arthien is talking about ankhers/mongodb#145. I would prefer to have the change that returns {:error, exception} instead of raising an exception when you try to query a disconnected connection.

If this is going to take a while though, I could look into rescuing instead of waiting on this.

christianjgreen commented 7 years ago

@ankhers Would you mind using rescue ! for now over tuples?

fishcakez commented 7 years ago

Unfortunately the latest commit we can release a patch version for is 53271f7d2636bafca3966bbee44d0ecf70d10064. I think there are still cases after that where DBConnection can raise instead of returning an error. I think all these are changed at some point so it might be we can create a v1.1 branch and you can add any further of these fixes required. It could be easily to rescue.

Unless limited to the DBConnection.Sojourn pool there are still other cases in master where an exit/1 can happen as part of anticipated operation so I think would always need catch exits also. Given that I believe this driver uses DBConnection for the ownership pool it will is likely required.

ankhers commented 7 years ago

I will test a version of mongodb with rescuing. I will look into doing this within the next couple days.

christianjgreen commented 7 years ago

@ankhers our dev team thanks you :)

leifg commented 7 years ago

Is there anything I can do to help? I would also be very interested in a new release.

fishcakez commented 6 years ago

We would need to get things working in Ecto.

@shdblowers @scouten are mssql and sqlite adapters about to detect transaction status (whether inside or outside a transaction) without a roundtrip across network? Obviously for SQLite it is just whether we can detect it as we're on same host.

IIRC neither of those adapter supports streaming so we don't need to worry about the other changes.

shdblowers commented 6 years ago

@fishcakez is that the work for supporting Ecto 2.2?

josevalim commented 6 years ago

@shdblowers not necessarily. Once we release a new db_connection version, I can backport any necessary fix to both Ecto 2.1 and Ecto 2.2 branches. So hopefully you are able to track those in isolation. The work I believe @fishcakez was referring to was to update to db_connection master.

shdblowers commented 6 years ago

Ah yes sorry, got confused between the two streams of work.

mssqlex is ready for the update to DBConnection.

fishcakez commented 6 years ago

@shdblowers the mssqlex PR is using the client, rather than the database, to set the transaction status: https://github.com/findmypast-oss/mssqlex/blob/9cee8944fa38f92cc2e000048fff232b8f5a1f75/lib/mssqlex/protocol.ex#L272. If ODBC doesn't (or can't) send the transaction status as part of the response then we shouldn't implement the feature because the client can get out of sync with the database if a user does query(.., "BEGIN...).

shdblowers commented 6 years ago

Hi @fishcakez , with ODBC transactions are handled at the connection level.

You can either be connected in auto-commit (each statement committed when sent) or manual-commit mode (everything is a transaction).

In that way the client and DB stay in sync with the status of a transaction.

However, ODBC does not send the transaction status as part of the response.

fishcakez commented 6 years ago

Oh, so it's not possible for a raw query to start a transaction or alter transaction state?

On 10 Oct 2017 10:36, "Steven Blowers" notifications@github.com wrote:

Hi @fishcakez https://github.com/fishcakez , with ODBC transactions are handled at the connection level.

You can either be connected in auto-commit (each statement committed when sent) or manual-commit mode (everything is a transaction).

In that way the client and DB stay in sync with the status of a transaction.

However, ODBC does not send the transaction status as part of the response.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-ecto/db_connection/issues/92#issuecomment-335550637, or mute the thread https://github.com/notifications/unsubscribe-auth/AB6JTVRDwqRvs_E4iEC4Hth7M4D0Y1JLks5sq6s7gaJpZM4O2cZc .

shdblowers commented 6 years ago

No, when connected in manual commit mode you can commit or rollback a transaction by calling :odbc.commit, not with a raw query.

fishcakez commented 6 years ago

Thank you @shdblowers, sounds good! Lets move forward with a release once the milestone issues are handled.