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

Can we run it? Yes we can! #19368

Merged
merged 1 commit into from
Dec 23, 2019
Merged

Can we run it? Yes we can! #19368

merged 1 commit into from
Dec 23, 2019

Conversation

ajcvickers
Copy link
Member

Design meeting feedback on #19334 for #6482

  • Formatting of the relational query string can now be overridden by providers
  • SQL Server overrides this to create runnable Parameter declarations
  • Tested by:
    • Executing the DbCommand we create when using CreateDbCommand
    • Executing a DbCommand created externally and given the generated query string

Design meeting feedback on #19334 for #6482

* Formatting of the relational query string can now be overridden by providers
* SQL Server overrides this to create runnable `Parameter` declarations
* Tested by:
  * Executing the DbCommand we create when using `CreateDbCommand`
  * Executing a DbCommand created externally and given the generated query string
@ajcvickers
Copy link
Member Author

@roji @lauxjpn @bricelam For SQL Server we now generate a query string that looks like this:

DECLARE @__city_0 nvarchar(4000) = N'London';

SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE [c].[City] = @__city_0

This can then be pasted into SQL Management Studio or similar to run and debug. Other providers could do the same thing.

@bricelam I had a quick look and didn't find this easy to do with SQLite. If there is a good way, then we should consider doing it.

@bricelam
Copy link
Contributor

bricelam commented Dec 19, 2019

SQLite CLI enables the following syntax:

.param set @__city_0 'London'

SELECT ...

@ajcvickers
Copy link
Member Author

ajcvickers commented Dec 19, 2019

@bricelam That's significantly simpler than what I found on the interwebs! This should be doable.

@lauxjpn
Copy link
Contributor

lauxjpn commented Dec 20, 2019

Nice one. That will work fine with MySQL. Should there be a statement terminator emitted at the end, or is it left out by design?

I guess it just depends on how the provider generates their commands and wants to output them in the end. So a statement terminator can just be added by the provider if needed when implementing IRelationalQueryStringFactory.

@roji
Copy link
Member

roji commented Dec 20, 2019

This will unfortunately be a bit more annoying on PostgreSQL, which doesn't support variables in regular SQL (although I can do an anonymous procedural code block with DO).

BTW we could just generate inlined literals instead of declaring variables and using them, but I suspect that would be more work than we intend to do for this (or is there another reason?)

@ajcvickers
Copy link
Member Author

@roji It's very important for SQL Server that we don't inline the parameters because this would result in a different query using a potentially different query plan and also potentially different plan cache hits/misses.

I'd argue even if this works for PostgreSQL it still wouldn't be using the same SQL that EF generates and hence would be misleading.

Also, experience in the past has shown this to be non-trivial (on SQL Server) because it's important that the literal be of exactly the same type as the parameter, which probably means ugly casting in the SQL, which then again defeats the purpose of being able to see the SQL generated by EF.

@ajcvickers
Copy link
Member Author

@roji Also, could you review this?

@roji
Copy link
Member

roji commented Dec 20, 2019

Thanks for the detailed response, am mainly trying to make sure I understand everything.

Also, experience in the past has shown this to be non-trivial (on SQL Server) because it's important that the literal be of exactly the same type as the parameter, which probably means ugly casting in the SQL, which then again defeats the purpose of being able to see the SQL generated by EF.

So if I understand correctly, instead of:

DECLARE @__city_0 nvarchar(4000) = N'London';

SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE [c].[City] = @__city_0

we think it's OK if we generated:

SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE [c].[City] = CAST(N'London' AS nvarchar(4000))

Right? But we think the latter option is both uglier, and possibly more difficult to implement (since we'd need a dedicated visitor for inlining parameters, used only for this purpose)?

Regardless will review in the next couple of days (holidays mode... :/)

@ajcvickers
Copy link
Member Author

@roji It would not be okay for SQL Server because the query planner and cache treats it differently. It may be okay technically for Npgsql--I don't know--but I still wouldn't do it because it's no longer showing the SQL that EF normally generates.

@roji
Copy link
Member

roji commented Dec 20, 2019

@ajcvickers thanks for the clarification. Sure, planner- and cache-wise I can see why there would be a difference, but I'd be surprised if there were any functional difference (i.e. non-perf, actual result difference) when comparing a fully typed (and faceted) literal vs. a parameter. But this isn't terribly important.

@ajcvickers
Copy link
Member Author

@roji I think perf differences are very important here. We have had many reports over the years where people have changed the SQL we generate in this way, run it in SQL Management Studio, and then reported that the perf is "fine" and therefore EF is doing something to make it slow.

@roji
Copy link
Member

roji commented Dec 20, 2019

Makes sense. If we want to retain the perf characteristics of queries, then for SQL Server it definitely makes sense to parameterize rather than using literals.

FYI this will probably be totally irrelevant for PostgreSQL, since AFAIK there's no transparent query cache as in SQL Server. If you want to reuse/cache query plans, you must explicitly prepare them (hence the importance of automatic preparation). I'll keep this in mind when implementing this for Npgsql (npgsql/efcore.pg#1173).

@ajcvickers
Copy link
Member Author

@bricelam Started looking at this for SQLite. It seems that the ".param" syntax isn't supported in the ADO.NET query string, which precludes this from the automated testing we're doing for SQL Server. Is this correct?

@bricelam
Copy link
Contributor

bricelam commented Jan 2, 2020

The ".param" syntax isn't supported in ADO.NET

Correct. It's a CLI-only feature. We could parse it in the tests 😏

@ajcvickers
Copy link
Member Author

@bricelam Is it worth it?

@bricelam
Copy link
Contributor

bricelam commented Jan 2, 2020

Parsing? No. Never.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants