Quote SQL identifiers only when necessary?

[roji]

In an effort to make EF Core-generated SQL as minimal and as readable as possible, I was considering quoting identifiers only when necessary. In other words, I'd override the appropriate methods in my ISqlGenerationHelper to check the content of the identifier and quote accordingly.

I was wondering if this has already been discussed... Of course the value of doing this isn't enormous, but if there's no major reason not to do it, then why not...

[ErikEJ]

Not sure I agree on readability if only some identifiers are quoted

[roji]

@ErikEJ really? You prefer all identifiers to be quoted, always, just because some of them may contain a space or some other character? Personally, when I look at SQL generated from EF Core I find myself wishing all the quote noise would be cleaned out of there...

[ErikEJ]

I do, yes! :-)

[roji]

OK, let's see what the others think :)

[smitpatel]

+1. I already do that in Cosmos Db provider. And the readability difference is rather large there.

[ErikEJ]

Good luck with that - rule number 3 (of five):

The identifier must not be a Transact-SQL reserved word. SQL Server reserves both the uppercase and lowercase versions of reserved words. When identifiers are used in Transact-SQL statements, the identifiers that do not comply with these rules must be delimited by double quotation marks or brackets. The words that are reserved depend on the database compatibility level. This level can be set by using the ALTER DATABASE statement.

https://docs.microsoft.com/en-us/sql/relational-databases/databases/database-identifiers

[austindrenski]

+1. This would be very useful.

To address reserved word concerns, how about making it opt-in via DbContext.OnConfiguring(...)?

public enum QuoteIdentifiers
{
    Always,
    AsRequired,
    Never
}

builder.QuoteIdentifiers(Always | AsRequired | Never)

[ErikEJ]

"Never" will break!

[roji]

@ErikEJ you're right that keywords are an issue. FWIW, the ADO.NET schema API has a collection which is supposed to provide the list of all reserved words. To what extent this is a reliable list on any given provider is another question... At least for PostgreSQL there's an official, hopefully exhaustive list,

[roji]

It's definitely not just a change you can slap on without looking at stuff like keywords, but it should be possible to do correctly...

[ErikEJ]

And doing it must not impact EF performance

[roji]

And doing it must not impact EF performance

Unless I'm mistaken these queries are cached, i.e. the delimiting process doesn't happen on every execution (but someone from the EF team should be able to confirm).

Aside from that, delimiting involves quite a bit of allocations for all the concatenations, which this would reduce.

[smitpatel]

Unless I'm mistaken these queries are cached, i.e. the delimiting process doesn't happen on every execution (but someone from the EF team should be able to confirm).

True. First level cache is based on ExpressionTree in linq. The same entry is used when parameter values are changed. Second level cache is at SQL tree level. Same entry is used as long as nullability of parameters does not change. Else we may need to generate a different SQL.

For Performance thing, we have #10513 to actually move most stuff out of SQL gen to first level cache so second level cache doesn't do much work apart from working with parameters.

[ajcvickers]

@roji @ErikEJ We discussed this in triage and we're not going to make any change to the providers we own primarily because it creates an additional coupling for reserved names, etc., but for your providers we're happy to do what you want. Please be sure that this is safe with regard to any kind of SQL injection attack--should be if it is limited to identifiers, but just saying...

[roji]

FYI, implemented this for Npgsql in npgsql/efcore.pg#327. The criteria for being an unquoted identifier in PostgreSQL is actually quite strict (lowercase letters, digits, underscore, dollar), and the reserved word list is pulled from the ADO.NET layer with GetSchema(). Only one keyword has been added in like 5 years, so it feels pretty safe :)