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

Documentation issue for EnableOptimizedParameterBinding #2393

Closed
Grauenwolf opened this issue Mar 7, 2024 · 6 comments · Fixed by #2417
Closed

Documentation issue for EnableOptimizedParameterBinding #2393

Grauenwolf opened this issue Mar 7, 2024 · 6 comments · Fixed by #2417
Labels
📃 Documentation Use this label for any documentation changes

Comments

@Grauenwolf
Copy link

If EnableOptimizedParameterBinding is true and you are calling a stored procedure with optional parameters, then the call may behave unexpectedly.

For example, say you stored procedure is

CREATE PROCEDURE dbo.UploadToMaxTest
	@AnsiLimted VarChar(500)  = null,
	@UniLimited NVarChar(1000)  = null,
	@Ansi VarChar(Max)  = null,
	@Uni NVarChar(Max)  = null
AS
[...]

And you want to pass in two parameters.

EXEC dbo.UploadToMaxTest @Ansi = 'AAAA', @Uni = N'UUUU';

If you turn on EnableOptimizedParameterBinding, then the generated SQL is...

EXEC dbo.UploadToMaxTest 'AAAA', N'UUUU';

Which means the wrong parameters will be used.

It should be called out in the documentation that EnableOptimizedParameterBinding is not compatible with missing optional parameters because the parameter names will be missing from the SQL.

@JRahnama JRahnama added this to Needs triage in SqlClient Triage Board via automation Mar 7, 2024
@JRahnama JRahnama added 📃 Documentation Use this label for any documentation changes and removed untriaged labels Mar 12, 2024
@JRahnama JRahnama moved this from Needs triage to Needs Investigation in SqlClient Triage Board Mar 12, 2024
@Wraith2
Copy link
Contributor

Wraith2 commented Mar 17, 2024

Can you tell us how you got the generated sql output? SqlClient doesn't send a string like that, instead it sends an RPC and as part of that RPC the parameters are passed and enabling optimized binding only prevents the name being sent. So the sql you're seeing is being generated by something and it's assuming that the names will be present.

If the parameter names are required for the sproc rpc type then it should be easy enough to disable them. could you write up a short test that shows this incorrect behaviour?

@Grauenwolf
Copy link
Author

I'm looking at the SQL Server profiler. So this is what the database is seeing regardless of how it got there.

@Grauenwolf
Copy link
Author

Code wise, I'm just using a normal SqlCommand, providing the proc name and two SqlParameter objects.

@Grauenwolf
Copy link
Author

and enabling optimized binding only prevents the name being sent

Exactly!

By not passing in the name, the database doesn't know that I'm skipping an optional parameter.

Which is fine so long as the documentation says that can happen.

@Wraith2
Copy link
Contributor

Wraith2 commented Mar 17, 2024

The reason for adding the ability to skip the names came from situations where there were hundreds to thousands of parameters which exposed the algorithm that the server uses when matching input and output names doesn't scale well. That situation can't occur with stored procedures unless you're writing one with hundreds to thousands of parameters which feels very unlikely.

I'm thinking we could just disable that option when you're executing a sproc, we could do it silently or throw an exception like we currently do it you try to use it with an out parameter.

@Grauenwolf
Copy link
Author

I'm thinking we could just disable that option when you're executing a sproc, we could do it silently

I think that's reasonable. It has a "pit of success" vibe to it which i appreciate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📃 Documentation Use this label for any documentation changes
Projects
Development

Successfully merging a pull request may close this issue.

3 participants