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

Consider adding a CommandBehavior parameter to RelationCommand methods. #33574

Closed
paulomorgado opened this issue Apr 19, 2024 · 9 comments
Closed
Assignees
Labels
area-query closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported type-enhancement

Comments

@paulomorgado
Copy link
Contributor

If one needs something other than CommandBehavior.Default, for example, CommandBehavior.SequentialAcccess, one needs to implement everything.

@roji
Copy link
Member

roji commented Apr 19, 2024

@paulomorgado CommandBehavior values significantly change how the command works - they're not typically something a user would be able to just "specify" without requiring significant adjustment on the EF side. For example, CommandBehavior.SequentialAccess require that columns be accessed in sequential order, and I'm not sure EF currently does that in all cases.

Could you provide a bit more context on what exact values of CommandBehavior you're looking for and why?

@paulomorgado
Copy link
Contributor Author

I'm looking specifically for CommandBehavior.SequentialAcccess to use in conjunction with #33569 to use DbDataReader.GetStream(Int32) Method with Microsoft.Data.SqlClient to read directly from the stream without incurring in a large byte[] allocation.

@roji
Copy link
Member

roji commented Apr 19, 2024

@paulomorgado give #6234 a thorough read, that's probably the issue you're looking for.

Note that just doing SequentialAccess doesn't help with reducing the large byte[] memory allocations - you need to actually stream the column value (SqlDataReader.GetStream()). EF can't do that for you - there are some fundamental mismatches between a fully streaming model (SequentialAccess) and an ORM which is supposed to materialize a user .NET type for you. This is probably a case where you want to simply drop down to ADO.NET rather than go through EF.

I'd also like to understand better how exactly you're expecting SequentialAccess and streaming to improve the allocation situation... In most cases where people need to work with large binary data, their application really does need to work with that data, and so there's no real way to avoid having the data in memory. Streaming can be helpful in various cases where e.g. the data needs to be dumped to a file or sent to another service, in which case that can happen in chunks thanks to streaming and not hold the entire data in memory. Is that your case?

@paulomorgado
Copy link
Contributor Author

Sometimes, one needs something that EFCore doesn't yet have that could be implemented closer to ADO.NET while benefiting from everything else like configuration, logging, pooling, migrations, etc. That's what I've been doing.

#6234 would probably work for what I need, but #33569 with #33574 would allow me to do that.

It's about empowering the user allowing for special cases when implementing it as a general case is not worth the development/maintenance.

I had to implement #33569 on my code to allow this and it's working fine, so far.

SequentialAccess could even be EF's default.

@roji
Copy link
Member

roji commented Apr 19, 2024

So again, just having some way to set CommandBehavior somewhere most probably does not help anyone - unless I'm missing something. Without GetStream(), you continue to have the same large allocations.

I'd advise not insisting on doing anything and everything through EF, but rather using the right tool for the job. You can perfectly well use EF, but drop down to ADO.NET for a specific query which needs to process large binary data - that would be my recommendation. This isn't dissimilar how people sometimes drop down to SQL queries when EF's LINQ translation can't handle some scenario (or produces sub-optimal SQL).

Specifically, introducing ADO.NET APIs into EF - which is #33569 is - really makes very little sense to me...

@paulomorgado
Copy link
Contributor Author

EF already has raw SQL execution.

This is just a more advanced scenario that is making life unnecessarily difficult when needed.

@roji
Copy link
Member

roji commented Apr 20, 2024

@paulomorgado if we indeed add APIs returning a DbDataReader as you're requesting in #33569, then adding CommandBehavior could make sense. But otherwise, EF is the one interacting with and consuming the DbDataReader, and allowing the user to specify CommandBehavior wouldn't make any sense, as far as I can tell.

Or am I missing something? Are you requesting a CommandBehavior API regardless of #33569?

@paulomorgado
Copy link
Contributor Author

There are other values, but I'm just thinking of SequentialAccess now.

@roji
Copy link
Member

roji commented Apr 29, 2024

Given that we've decided not to add execution APIs that return DbDataReader (#33569), we don't think there's any value in allowing the user to specify CommandBehavior, since the consumer of DbDataReader must react accordingly, and that's EF.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Apr 29, 2024
@roji roji added the closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants