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

UDT parameters are limited to 64K #329

Closed
ajcvickers opened this issue Nov 24, 2019 · 25 comments · Fixed by #340 or #456
Closed

UDT parameters are limited to 64K #329

ajcvickers opened this issue Nov 24, 2019 · 25 comments · Fixed by #340 or #456
Labels
💡 Enhancement New feature request

Comments

@ajcvickers
Copy link
Member

This is an issue because serialized spatial types can easily be bigger than this. See dotnet/efcore#18813.

@Wraith2 pointed to the place in the code where this limitation exists:

//it may be legitimate, but we dont support it yet
if (size < 0 || (size >= ushort.MaxValue && maxsize != -1))
throw new IndexOutOfRangeException();

/cc @bricelam

@cheenamalhotra
Copy link
Member

It seems that the driver can support larger data size for UDT Types (upto 2 GB, i.e. max LOB size, identified by 0x7FFFFFFF token with length as -1, identified by 0xFFFF token), as introduced in TDS 7.3, but the support was not added to the driver.

Quoting MS TDS:

The value 0xFFFF signifies the maximum LOB size indicating a UDT with a maximum size greater than 8000 bytes (also referred to as a Large UDT; introduced in TDS 7.3).

This applies to both .NET Framework and .NET Core.
Issue would be categorized as an enhancement.

@cheenamalhotra cheenamalhotra added the 💡 Enhancement New feature request label Nov 27, 2019
@Wraith2
Copy link
Contributor

Wraith2 commented Nov 27, 2019

That shouldn't be too hard to fix. If i get time i'll have a go. I'd also like to change the exception type that it throws and give a sensible message to the user explaining why they're getting the error. A default IndexOutOfRange exception makes it look like an internal problem (fooled me when i was investigating) rather than something the user can address.

@Wraith2
Copy link
Contributor

Wraith2 commented Nov 29, 2019

How do you get the connection tds protocol version?There's a major+minor version on the session but that doesn't seem to get used. There's also _isYukon _isKatmai _isDenali on the parser but I'm not sure how they map to protocol versions.

@cheenamalhotra
Copy link
Member

@Wraith2

TDSParser when parses tokens calls this method for LOGINACK token:

internal void OnLoginAck(SqlLoginAck rec)
{
_loginAck = rec;
if (_recoverySessionData != null)
{
if (_recoverySessionData._tdsVersion != rec.tdsVersion)
{
throw SQL.CR_TDSVersionNotPreserved(this);
}
}
if (_currentSessionData != null)
{
_currentSessionData._tdsVersion = rec.tdsVersion;
}
}

@aprognimak
Copy link

Ehm, guys, any workaround (no matter how ugly)?
This used to work in 2.2 and it really is not an enhancement but a breaking bug

@Wraith2
Copy link
Contributor

Wraith2 commented Dec 2, 2019

It didn't work in 2.2. It's been this way since the inception of the feature, the only thing that can have changed is the amount of data you feed it. I'm working my way through the fix but I've got a few things going on at the minute so it's a bit slow.

@ajcvickers
Copy link
Member Author

@bricelam Is it possible that EF Core didn't hit this in 2.2 because it was using a different mechanism that didn't hit this exact code path? (I don't remember exactly what we changed here.)

@aprognimak
Copy link

aprognimak commented Dec 2, 2019

It didn't work in 2.2. It's been this way since the inception of the feature, the only thing that can have changed is the amount of data you feed it. I'm working my way through the fix but I've got a few things going on at the minute so it's a bit slow.

Well it did. Just triple hours ago I was on 2.2 using the olden NetTopologySuite types, those IGeometry and all that stuff. However saving changes to the context when using spatial data even when there were no changes took enormous time (minutes for 1k+ shapes, unchanged) so I moved to 3.0.1 and it just stopped working.
Now I can't reseed my db with us state county borders because even with the least accuracy some of those don't fit into 64k parameter value. 2.2 most likely used quite different approach when dealing with spatial data given it was just introduced then.

Trying postgres now.

@capesean
Copy link

capesean commented Dec 3, 2019

Ehm, guys, any workaround (no matter how ugly)?
This used to work in 2.2 and it really is not an enhancement but a breaking bug

I’m using ExecuteSqlRaw with geometry::STGeomFromText('{geometry.ToString()}' as a temporary fix.

@bricelam
Copy link

bricelam commented Dec 3, 2019

EF Core didn't hit this in 2.2 because it was using a different mechanism

Correct. We were using SqlBytes parameters (not UDTs) in 2.2. We switched to UDTs in 3.0 to fix dotnet/efcore#14595

@ajcvickers
Copy link
Member Author

@cheenamalhotra I think this raises the priority of getting this changed, since without it customers using spatial with EF Core will be running into blocking issues. Hopefully @Wraith2 will come up with a fix, but if not can we raise the priority of getting this in?

@Wraith2
Copy link
Contributor

Wraith2 commented Dec 4, 2019

Prospective fix is posted #340

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Dec 5, 2019

Thanks for the PR @Wraith2

Is there a special reason to increase size limit to int.MaxValue? I'm wondering if it would bring back the same support as sending data as SqlBytes previously with .NET Core 2.2.

The size we're looking at is 2 GB LOBs that driver needs to support with Large UDTs.

@cheenamalhotra
Copy link
Member

Ok, I see the driver does support Large UDTs, it was probably not hitting the right code path when working with large data. I'll continue to review PR and post comments there, just want to make sure the metadata information is available as per specs.

@Wraith2
Copy link
Contributor

Wraith2 commented Dec 5, 2019

It can already receive large UDTs but the check in sending them was hardcoded to short.MaxValue which as you pointed out earlier in this discussion was only for 2005. I just made the check version aware.

Since sending the UDT doesn't require you to send the size field for the UDT description this check could be omitted entirely but if a user did go over the limit they would get an SqlException back from the server after having had to wait for transferring>2Gib and it wouldn't be a very friendly message.

@loth0
Copy link

loth0 commented Dec 12, 2019

Any hint on when this fix will show up in ef core? This is what's blocking us from upgrading to ef core 3

@Wraith2
Copy link
Contributor

Wraith2 commented Dec 12, 2019

Do you need it on netcore or desktop? this fix is only netcore at the moment, it's probably possible to get it into desktop as well if the code in that version is pretty similar.

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 12, 2019

If it is for EF Core, it will always be the .NET Core SqlClient

@cheenamalhotra
Copy link
Member

Hi @loth0

Our next preview release containing the fix is planned in mid-January, after which you should be able to custom import the new preview Microsoft.Data.SqlClient package in your application with EF Core 3.1.

EF Core should in future upgrade the dependency version for Microsoft.Data.SqlClient, but for their timelines you'll need to check with them.

@jsgoupil
Copy link

@cheenamalhotra When will we know when this is fixed? Because I have EF Core 3.1.1 and I can still repro this problem.

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Jan 28, 2020

@jsgoupil

Since the fix was released in v2.0.0-preview1 of Microsoft.Data.SqlClient, you need to add custom reference to this version to get this fix:

<PackageReference Include="Microsoft.Data.SqlClient" Version="2.0.0-preview1.20021.1" />

@juliusfriedman
Copy link

See also NetTopologySuite/NetTopologySuite#383, it seems the work around is to use a smaller string for now.

@loth0
Copy link

loth0 commented Aug 31, 2020

Have this been included in latest EF Core (3.1.7)? i.e can I remove my package reference to Microsoft.Data.SqlClient?

@cheenamalhotra
Copy link
Member

Hi @loth0

EF Core seems to use SqlClient v2.0.0 starting 5.0.0-preview versions:
https://www.nuget.org/packages/Microsoft.EntityFrameworkCore.SqlServer/5.0.0-preview.7.20365.15

While v3.1.7 depends on M.D.S v1.1.3 that does not contain this fix.

@roji
Copy link
Member

roji commented Aug 31, 2020

@loth0 it should be fine to use EF Core 3.1.7 and to take an explicit dependency on SqlClient 2.0.1 - that version just isn't brought in by default, that's all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement New feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.