Add support for INCLUDE clause in indexes by khellang · Pull Request #699 · npgsql/efcore.pg

khellang · 2018-11-14T18:34:48Z

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 · 2018-11-15T17:45:04Z

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

khellang · 2018-11-15T17:47:47Z

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 · 2018-11-15T17:56:00Z

@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 · 2018-11-15T18:25:59Z

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 · 2018-11-15T18:29:26Z

Filed #704 for scaffolding support.

khellang · 2018-11-15T19:46:36Z

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

roji · 2018-11-15T20:09:56Z

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 · 2018-11-15T21:00:05Z

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 · 2018-11-15T21:24:26Z

@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 · 2020-09-23T08:34:29Z

 .Append(IndexColumnList(operation.Columns, operators))
 .Append(")");

- if (!string.IsNullOrEmpty(operation.Filter))


@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?

/CC: @khellang

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).

@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

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.

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

@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?

You could also try to just modify the model itself, instead of swapping components? I.e.
protectedoverridevoidOnModelCreating(ModelBuildermodelBuilder){foreach(varentityinmodelBuilder.Model.GetEntityTypes()){foreach(varindexinentity.GetDeclaredIndexes()){if(index.IsUnique){index.SetFilter("is_deleted is FALSE");}}}}

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

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

khellang force-pushed the index-include branch 2 times, most recently from d3e848a to 38f4885Compare November 14, 2018 18:49

Add support for INCLUDE clause in indexes
e967fda

khellang force-pushed the index-include branch from 38f4885 to e967fdaCompare November 14, 2018 18:50

khellang commented Nov 14, 2018
View reviewed changes

Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

khellang commented Nov 14, 2018
View reviewed changes

Comment threadsrc/EFCore.PG/Metadata/INpgsqlIndexAnnotations.cs

austindrenski requested review from YohDeadfall, austindrenski and roji November 14, 2018 20:13

austindrenski added the enhancement label Nov 14, 2018

austindrenski suggested changes Nov 15, 2018
View reviewed changes

Comment threadsrc/EFCore.PG/Extensions/NpgsqlIndexBuilderExtensions.cs
Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs Outdated

roji approved these changes Nov 15, 2018
View reviewed changes

Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs Outdated
Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs Outdated
Comment threadsrc/EFCore.PG/Metadata/INpgsqlIndexAnnotations.cs
Comment threadsrc/EFCore.PG/Design/Internal/NpgsqlAnnotationCodeGenerator.cs

YohDeadfall approved these changes Nov 15, 2018
View reviewed changes

Use pattern matching
1da5630

austindrenski approved these changes Nov 15, 2018
View reviewed changes

Add XML docs
6eda95c

khellang force-pushed the index-include branch from 778aa91 to 6eda95cCompare November 15, 2018 18:25

khellang mentioned this pull request Nov 15, 2018
Add scaffolding support for new index features #704
Closed

Add Npgsql-specific method to get generic IndexBuilder
81ccc5c

khellang mentioned this pull request Nov 15, 2018
Replace ForSqlServerHasIndex with HasIndex dotnet/efcore#12366
Closed

austindrenski suggested changes Nov 15, 2018
View reviewed changes

Address PR feedback
53bdc8c

austindrenski approved these changes Nov 15, 2018
View reviewed changes

roji merged commit c1b5cdc into npgsql:devNov 16, 2018

khellang deleted the index-include branch November 16, 2018 06:18

khellang mentioned this pull request Feb 18, 2019
Clean up support for expression-based index INCLUDE #813
Closed

suadev reviewed Sep 23, 2020
View reviewed changes

Uh oh!

Conversation

khellang commented Nov 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

austindrenski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

khellang commented Nov 15, 2018

Uh oh!

khellang commented Nov 15, 2018

Uh oh!

austindrenski commented Nov 15, 2018

Uh oh!

khellang commented Nov 15, 2018

Uh oh!

khellang commented Nov 15, 2018

Uh oh!

khellang commented Nov 15, 2018

Uh oh!

roji commented Nov 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khellang commented Nov 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

austindrenski commented Nov 15, 2018

Uh oh!

austindrenski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suadevSep 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suadevSep 23, 2020

Choose a reason for hiding this comment

Uh oh!

YohDeadfallSep 23, 2020

Choose a reason for hiding this comment

Uh oh!

khellangSep 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojiSep 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojiSep 23, 2020

Choose a reason for hiding this comment

Uh oh!

suadevSep 23, 2020

Choose a reason for hiding this comment

Uh oh!

khellangSep 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suadevSep 23, 2020

Choose a reason for hiding this comment

Uh oh!

khellang commented Nov 14, 2018•
edited
Loading

roji commented Nov 15, 2018•
edited
Loading

khellang commented Nov 15, 2018•
edited
Loading

suadevSep 23, 2020•
edited
Loading

khellangSep 23, 2020•
edited
Loading

rojiSep 23, 2020•
edited
Loading

khellangSep 23, 2020•
edited
Loading