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

Improving support of nvarchar and nvarbinary on the ODBC driver #4822

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HernanRivasAcosta
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented May 12, 2021

CLA assistant check
All committers have signed the CLA.

@rickard-green rickard-green added the team:PS Assigned to OTP team PS label May 15, 2021
@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label May 18, 2021
@IngelaAndin
Copy link
Contributor

There are a lot of white space changes it would be good if we could have them in a separate commit to make reviewing easier.

lib/odbc/vsn.mk Outdated
@@ -1 +1 @@
ODBC_VSN = 2.13.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be updated manually!

_ ->
postgres
Copy link
Contributor

Choose a reason for hiding this comment

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

This i not going to work out for the nightly builds! Also it eliminates all Postges test which probably is not a good thing.

@IngelaAndin IngelaAndin added waiting waiting for changes/input from author and removed testing currently being tested, tag is used by OTP internal CI labels May 20, 2021
@IngelaAndin IngelaAndin self-assigned this May 21, 2021
@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Jun 1, 2021
@@ -27,7 +27,7 @@

%-------------------------------------------------------------------------
connection_string() ->
"DSN=sql-server;UID=odbctest;PWD=gurka".
"DSN=hernan-mssql;UID=hernan;PWD=hernan".
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not ok to change the basic test setup!

@IngelaAndin IngelaAndin added help wanted Issue not worked on by OTP; help wanted from the community and removed stalled waiting for input by the Erlang/OTP team labels Mar 29, 2023
@IngelaAndin
Copy link
Contributor

Nothing happened here for a long time. There is no priority to drive this change from the OTP team but we are willing to accept it if any one is interested in making it work!

@rnowak
Copy link
Contributor

rnowak commented Apr 18, 2023

Hi,

The issue involving the large data types specified with a max size has been a growing pain for us lately. I've together with a colleague looked at this patch and there is an off by one error somewhere in calculating the chunking or the buffer. We'll see if we can dig further into it and whatever else we need to do to land this upstream.

Additionally, one of the recent PRs (#7000) for odbcserver causes it to misbehave when used towards a Microsoft SQL Server database, where such data types with max size are specified. Before the recent PR, things worked "fine" as long as one used the ancient {SQL Server} driver -- anything newer such as {ODBC Driver 18 for SQL Server} also causes issues. This PR would probably make that good again.

As a side note, I'll be preparing a PR that allows for multiple result sets when using parameterized queries unless you can think of some reason why that shouldn't be done.

Cc: @micke314 @aronisstav @anders-ramsell-telia

@IngelaAndin
Copy link
Contributor

@rnowak It was many years ago that I worked actively to improve this application. I think not handling multiple result sets is probably a missing feature that was never prioritized. We do apricate that being a separate PR. Any hint on how to set up a working test environment on a modern windows environment would also be greatly apricated, I am afraid ours has gone rotten and Postgress is our only working backend at the moment.

@IngelaAndin IngelaAndin added the review_help_wanted Help with review wanted label Oct 26, 2023
@IngelaAndin IngelaAndin added enhancement feature and removed waiting waiting for changes/input from author enhancement labels May 28, 2024
@IngelaAndin IngelaAndin removed their assignment May 28, 2024
@rnowak
Copy link
Contributor

rnowak commented May 28, 2024

Just a small status update that this is still on my table, it has just been shifted off constantly due to other prioritization. The hope is to have this done before the end of this year. The merged #7000 in combination with only the ancient {SQL Server} driver being usable right now is a pretty large blocker and is preventing us from upgrading to a more recent Erlang version in addition to being able to migrate to Linux.

Not sure how to best proceed, but perhaps it might be better to close this PR and start afresh when it is time. This PR includes a lot of mixed changes, and ultimately, the chosen approach cannot work with the ODBC drivers towards Microsoft SQL Server in certain scenarios as it goes against specific requirements outlined in the Microsoft documentation. It is illegal to retrieve bound columns after a call to SQLGetData -- it causes empty values to be returned silently.

I have an implementation prepared, but it needs some more love. It changes how variable-sized types are retrieved, and this will hold true for all database kinds, not only Microsoft SQL Server. Ultimately, this should match what the drivers might have performed under the hood otherwise, and hopefully it will have similar performance characteristics. I will try to verify my hopes towards PostgreSQL.

I want to reiterate that the PR as it is currently should not be merged.

@IngelaAndin
Copy link
Contributor

@rnowak You are of course still most welcome to do the work you intended on this one. I fully agree it should not be merged as it is. If you feel it would be easier to make a new PR please do, and when you do we can close this one in favor of the new one.
The reason we changed it was that we are cleaning up after OTP-27 release and we short of gave up hope on the waiting label as we have note heard from you in a long time. I also added the review help wanted to enable more interested people to perhaps come with valuable input, just approving it as it is now is however not helpful to the cause if it being mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted Issue not worked on by OTP; help wanted from the community priority:low review_help_wanted Help with review wanted team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERL-421: ODBC truncates data for LONGTEXT and BLOB
6 participants