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

Experimental Sybase support #3

Closed

Conversation

kizzx2
Copy link

@kizzx2 kizzx2 commented Apr 20, 2017

No description provided.

@shdblowers
Copy link
Collaborator

Hello, @kizzx2! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@coveralls
Copy link

coveralls commented Apr 20, 2017

Coverage Status

Coverage decreased (-0.6%) to 83.871% when pulling c824998 on kizzx2:sybase-quoted-identifier into 15c3d5d on findmypast-oss:master.

@jbachhardie
Copy link

Thanks for the PR.

I'd say this seems like an entirely reasonable addition, but I'd like to see the integration test suite ran against Sybase before we advertise support for it. Sybase could be run in a container just like MS SQL Server is currently and tested against on Travis.

@jbachhardie
Copy link

Another thing, if you could add the ability to specify extra connection string parameters like TDS_Version as, say, a map that gets converted in Protocol.connect/1 that would be better than hacking that in by appending it to the end of hostname.

@kizzx2
Copy link
Author

kizzx2 commented Apr 21, 2017

@jbachhardie Sure, I am trying to use mssqlex in my work with a production Sybase and would try to clean up this PR when I get the time, along with some other things that I discover along the way.

Regarding the option for "advanced options" like TDS_Version, given the plethora of options one can actually add, and the norm of specifying a connection string when using ODBC how about just add a connection_string parameter that overrides other options when it's present? Probably easier than adding an option for each of them.

@jbachhardie
Copy link

We could have an additional_options parameter that takes a map, i.e. %{"TDS_Version" => "5.0", "TextSize" => 2000, ...} which gets parsed into connection string parameters and appended at the end of the other parameters. That way people can use the standard params and extra ones at the same time, and it can all be expressed in native Elixir syntax rather than connection string syntax.

@kizzx2
Copy link
Author

kizzx2 commented Jun 11, 2017

Just wanted to check in, We switched over to use erlangbureau/jamdb_sybase instead, due to a fatal hardcoded constraint in Erlang's ODBC implementation.

Basically, Erlang ODBC will silently truncate any value fields that's longer than 8KB. That became a deal breaker for us.

I am not sure whether this issue can be circumvented when dealing with MSSQL, but we wasted enough time fighting with it for our Sybase project.

I can finish this PR for the options part but probably will not be able to invest the time needed to get test coverage and fix all the test cases.

What do you think?

@shdblowers
Copy link
Collaborator

Hi @kizzx2 sorry for not getting back sooner.

The options functionality sounds like a worthwhile addition, if you're willing to code it, we're willing to review/accept it.

However, we won't accept the sybase support code without proper test coverage.

@shdblowers
Copy link
Collaborator

Closing this due to inactivity.

@shdblowers shdblowers closed this Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants