Add support for INCLUDE clause in indexes

[khellang]

Closes #697

[austindrenski]

A few inline comments, but looks good so far.

One note is that if we do change the naming, we need to make sure it's consistent throughout the PR.

[roji]

Looks good from my side, but let's see what the others think.

Are you planning to work on scaffolding support for this as well? It would be nice to release full support for this feature at the same time. If so, let's include it in this PR, otherwise can you please open a separate issue to track? Maybe I'll take a stab at it.

[khellang]

I'll address PR feedback and add XML comments tonight, then you can have a final review pass 👍🏻

[khellang]

Are you planning to work on scaffolding support for this as well? It would be nice to release full support for this feature at the same time. If so, let's include it in this PR, otherwise can you please open a separate issue to track? Maybe I'll take a stab at it.

We punted on it for #662. Maybe we can create an issue and tackle both at the same time? You probably need to dig into some of the same metadata anyway.

[austindrenski]

@khellang Not to pile on more index work.... but I wanted to bring #119 and #326 to your attention, in case they influence any of your work here, or the proposed scaffolding work.

[khellang]

I think we're mostly there. The only unresolved issue, as far as I can see, is whether we should do a VersionAtLeast check or not...

[khellang]

Filed #704 for scaffolding support.

[khellang]

I have a PR lined up for fixing #326 once this is merged 😄

[roji]

Are you planning to work on scaffolding support for this as well? It would be nice to release full support for this feature at the same time. If so, let's include it in this PR, otherwise can you please open a separate issue to track? Maybe I'll take a stab at it.

We punted on it for #662. Maybe we can create an issue and tackle both at the same time? You probably need to dig into some of the same metadata anyway.

Ah OK, understood. Sure, can you please create an issue for doing both? Ideally we'd still have two separate commits, but as you say it makes sense to work on the two at the same time. Would you be interested and have the time to work on this in the very near future? If not I can probably do it.

EDIT: Just saw #704 for the scaffolding support, thanks!

[khellang]

I added an Npgsql-specific method to return a generic IndexBuilder<TEntity>, which will allow you to use an expression for defining the included columns.

According to dotnet/efcore#4846 (comment), we might be able to obsolete it once 3.0 ships with an official HasIndex that returns the generic version:

For 3.0, we should consider breaking HasIndex to return the generic and obsoleting ForSqlServerHasIndex.

It's being tracked by dotnet/efcore#12366

[austindrenski]

@khellang wrote:

I think we're mostly there. The only unresolved issue, as far as I can see, is whether we should do a VersionAtLeast check or not...

Per the inline discussion above, no need to add the check. It would indeed introduce a bit too much hidden magic without adding much in value terms.

@khellang wrote:

I added an Npgsql-specific method to return a generic IndexBuilder<TEntity>, which will allow you to use an expression for defining the included columns.

According to dotnet/efcore#4846 (comment), we might be able to obsolete it once 3.0 ships with an official HasIndex that returns the generic version:

Nice!

[austindrenski]

The implementation looks good. Just some nits in the XML docs

[suadev]

@roji hi,

I don't know why this 'Where' is deleted but i can't add a filter for my all indexes. I need to add this index filter; 'is_deleted is FALSE' just because i am using soft delete with unique indexes. Any idea?

[suadev]

/CC: @khellang

[YohDeadfall]

INCLUDE isn't the same as the WHERE option. The feature which @khellang added supports the first one to improve read performance (the specified columns are included into the index, but not participate in ordering; this allows to take values only from one table called an index table without joining to the original table).

[khellang]

@suadev It's deleted because it's using the overriden IndexOptions method, with a call to base.IndexOptions below.

If you take a look at the base class (in EF Core), you can see that it's still adding the WHERE clause if there's a filter specified:

https://github.com/dotnet/efcore/blob/c13f8b4a645c6b7cf141c9c62833ea4269622912/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs#L1832-L1843

[roji]

To add to what @YohDeadfall wrote, the WHERE support wasn't deleted - it was simply moved into EF Core itself (see MigrationsSqlGenerator).

Are you seeing an actual issue with it not working? If so, please open a new issue with a code sample so we can investigate.

[roji]

You may need to look at this: https://docs.microsoft.com/en-us/ef/core/miscellaneous/configuring-dbcontext#design-time-dbcontext-configuration

[suadev]

@roji thanks, but i didn't get it. I am still not sure what is my mistake. Overriding IndexOptions method is not a correct way?

[khellang]

You could also try to just modify the model itself, instead of swapping components? I.e.

protected override void OnModelCreating(ModelBuilder modelBuilder) {
	foreach (var entity in modelBuilder.Model.GetEntityTypes()) {
		foreach (var index in entity.GetDeclaredIndexes()) {
			if (index.IsUnique) {
				index.SetFilter("is_deleted is FALSE");
			}
		}
	}
}

[suadev]

@khellang thank you! i am not even aware of GetDeclaredIndexes() method !

[roji]

Changing the model is definitely a better approach than replacing services, wherever it's possible.