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

Default Size behavior in RelationalTypeMapping may be incorrect #11591

Closed
roji opened this issue Apr 8, 2018 · 5 comments
Closed

Default Size behavior in RelationalTypeMapping may be incorrect #11591

roji opened this issue Apr 8, 2018 · 5 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Apr 8, 2018

Originally raised in npgsql/efcore.pg#357.

The default behavior in RelationalTypeMapping is to set the parameter's Size to the mapping's Size, if one is specified. Since DbParameter.Size means truncating the value, that means that by default EF Core tells the underlying ADO.NET provider to perform client-side truncation based on the mapping's size. This behavior isn't overridden in StringTypeMapping.

However, the SqlServer provider does override this behavior for strings and for byte arrays. Both seem to set DbParameter.Size to either maxSpecificSize (if the value length is smaller than then max column length) or to -1. This way, the parameter data is never truncated and the backend returns an error as expected.

If this is the behavior that you're looking for across providers (and I see no reason for this to vary), then I think you may want to change RelationalTypeMapping to no longer truncate by default.

@ajcvickers
Copy link
Member

@roji The code for setting the size in SQL Server is there for a number of reasons:

  • Avoid fragmentation of query plans on the server
  • Make sure data is not truncated when the real size of the column is not known
  • Still allow the server to throw if the data does not fit

The key part about the first point is to use a small number of size values for parameters and to use the correct size whenever possible. Therefore:

  • If the size of the column has been specified, then use this for the parameter size
  • Otherwise use the max size that can typically be handled outside of a (max) column
  • Unless in either of these cases value being saved wouldn't fit, in which case use -1 for size to stop SQLClient from doing its own size inference.

On the other hand, the default behavior for parameters is to set size only if the size has been specified, otherwise do nothing. This seemed like a good default behavior given that different ADO.NET providers can behave differently and have different requirements for size.

So my question is, based on your experience, which parts of the SQL Server algorithm do you think are common across providers? For example, does Postgres ever require the size to be set? If so, what is the impact of setting the size?

@ajcvickers
Copy link
Member

@roji Any thoughts on this?

@roji
Copy link
Member Author

roji commented Apr 14, 2018

It'll take me a few more days to respond, sorry...

@roji
Copy link
Member Author

roji commented Apr 18, 2018

So my question is, based on your experience, which parts of the SQL Server algorithm do you think are common across providers? For example, does Postgres ever require the size to be set? If so, what is the impact of setting the size?

On the PostgreSQL side, there's not a whole lot of usage for length. Unless I'm mistaken, the only column types where length is allowed are text and bitstring. Actually specifying the size isn't required: varchar without parentheses is legal, and means that any length is supported (much like text). The same is true of bit varying, which is an arbitrary-length bitstring. In addition, the docs for string types seem to explicitly say that there's no perf impact for specifying the length. In other words, length seems to be an optional feature that's only there to allow database validation of data, and has no other consequence.

On the client side, as I wrote before, setting DbParameter.Size seems to mean "truncate my value to this size when sending to the database". I understand that the actual behavior may vary across different providers, and that it's conceivable that some provider out there requires you to set DbParameter.Size to the same value as your column's configured size - but I haven't seen anything like this. It's definitely not the case for PostgreSQL, and I think it would be against the behavior described in docs.

To summarize, if you don't want to stand in the way of server validation of length (which is how your SQL Server provider works at the moment), then it may be a good idea to stop setting DbParameter.Size. The mapping's size would of course still be used when creating the column etc., but would no longer impact how parameter data is actually sent by the ADO.NET provider. On the other hand, if you have reason to believe that some providers out there treat DbParameter.Size as something other than client truncation, then the current behavior may be fine.

@ajcvickers
Copy link
Member

Thanks @roji. We decided in triage that we will make the change in 2.2 to never set Size by default. SQL Server will continue to override for its additional logic. Currently Oracle, MySQL and SqlCompact also all override and so will not be impacted by the change. It will cause Postgres and SQLite to stop silently truncating, which could be considered a breaking change, but we are choosing to view as a bug fix. You may want to override in the Postgres provider for 2.1--it's a safer change to make there at this point than it is to make it in the core code.

@ajcvickers ajcvickers added this to the 2.2.0 milestone Apr 18, 2018
@ajcvickers ajcvickers self-assigned this May 23, 2018
@ajcvickers ajcvickers modified the milestones: 2.2.0-preview2, 2.2.0 Sep 11, 2018
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Sep 13, 2018
roji added a commit to roji/efcore.pg that referenced this issue Dec 12, 2018
We used to have our special NpgsqlStringTypeMapping, extending
StringTypeMapping, in order to override the Size behavior
of the latter. The problematic was removed in 2.2.0 (see
dotnet/efcore#11591),
so we no longer need this.
roji added a commit to npgsql/efcore.pg that referenced this issue Dec 14, 2018
We used to have our special NpgsqlStringTypeMapping, extending
StringTypeMapping, in order to override the Size behavior
of the latter. The problematic was removed in 2.2.0 (see
dotnet/efcore#11591),
so we no longer need this.
@ajcvickers ajcvickers modified the milestones: 2.2.0-preview3, 2.2.0 Nov 11, 2019
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

2 participants