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

Scaffold error when two indexes exist on the same column set #11846

Closed
roji opened this issue Apr 28, 2018 · 23 comments · Fixed by #25917
Closed

Scaffold error when two indexes exist on the same column set #11846

roji opened this issue Apr 28, 2018 · 23 comments · Fixed by #25917
Assignees
Labels
area-scaffolding closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-2.2 punted-for-3.0 type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Apr 28, 2018

In npgsql/efcore.pg#228, @berets76 describes an issue with the way indices are managed during scaffolding.

PostgreSQL supports multiple index "methods", and the Npgsql provider represents these via a simple string annotation on the index - all indices in a scaffolded DatabaseModel contain this annotation (note that if the default method is detected, the annotation is later eliminated by convention in NpgsqlAnnotationCodeGenerator).

Now, consider the following database schema:

CREATE TABLE data
(
  id integer PRIMARY KEY,
  name character(3)
);

CREATE INDEX index1 ON data (name);
CREATE INDEX index2 ON data (name DESC);

Two indices exist on the same column, which should be fine. However, trying to scaffold this throws the following:

System.InvalidOperationException: The annotation 'Npgsql:IndexMethod' cannot be added because an annotation with the same name already exists.
   at Microsoft.EntityFrameworkCore.Infrastructure.Annotatable.AddAnnotation(String name, Annotation annotation)
   at Microsoft.EntityFrameworkCore.MutableAnnotatableExtensions.AddAnnotations(IMutableAnnotatable annotatable, IEnumerable`1 annotations)
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitIndex(EntityTypeBuilder builder, DatabaseIndex index) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Scaffolding\Internal\RelationalScaffoldingModelFactory.cs:line 607
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitIndexes(EntityTypeBuilder builder, ICollection`1 indexes) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Scaffolding\Internal\RelationalScaffoldingModelFactory.cs:line 560
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitTable(ModelBuilder modelBuilder, DatabaseTable table) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Scaffolding\Internal\RelationalScaffoldingModelFactory.cs:line 323
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitTables(ModelBuilder modelBuilder, ICollection`1 tables) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Scaffolding\Internal\RelationalScaffoldingModelFactory.cs:line 279
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.VisitDatabaseModel(ModelBuilder modelBuilder, DatabaseModel databaseModel) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Scaffolding\Internal\RelationalScaffoldingModelFactory.cs:line 182
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.RelationalScaffoldingModelFactory.Create(DatabaseModel databaseModel, Boolean useDatabaseNames) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Scaffolding\Internal\RelationalScaffoldingModelFactory.cs:line 99
   at Microsoft.EntityFrameworkCore.Scaffolding.Internal.ReverseEngineerScaffolder.ScaffoldModel(String connectionString, IEnumerable`1 tables, IEnumerable`1 schemas, String namespace, String language, String contextDir, String contextName, ModelReverseEngineerOptions modelOptions, ModelCodeGenerationOptions codeOptions) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Scaffolding\Internal\ReverseEngineerScaffolder.cs:line 97
   at Microsoft.EntityFrameworkCore.Design.Internal.DatabaseOperations.ScaffoldContext(String provider, String connectionString, String outputDir, String outputContextDir, String dbContextClassName, IEnumerable`1 schemas, IEnumerable`1 tables, Boolean useDataAnnotations, Boolean overwriteFiles, Boolean useDatabaseNames) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Design\Internal\DatabaseOperations.cs:line 94
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.ScaffoldContextImpl(String provider, String connectionString, String outputDir, String outputDbContextDir, String dbContextClassName, IEnumerable`1 schemaFilters, IEnumerable`1 tableFilters, Boolean useDataAnnotations, Boolean overwriteFiles, Boolean useDatabaseNames) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Design\OperationExecutor.cs:line 463
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.ScaffoldContext.<>c__DisplayClass0_1.<.ctor>b__0() in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Design\OperationExecutor.cs:line 445
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.<>c__DisplayClass3_0`1.<Execute>b__0() in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Design\OperationExecutor.cs:line 555
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action) in C:\b\w\33bdfc1cae7b2a38\modules\EntityFrameworkCore\src\EFCore.Design\Design\OperationExecutor.cs:line 538

It seems that at some point during the scaffolding process, the two indices provided by the DatabaseModel get "collapsed" to one, and we get the error since the annotation is already set. This is probably because at some point EF Core looks up the index, keying only by the column list, and omits other index characteristics (e.g ascending/descending).

Thanks again to @berets76 for reproducing and analyzing, I only provided the writeup.

@smitpatel
Copy link
Contributor

#4150 Index column sort order is not supported in metadata at all. Hence, the scaffolded model did not expect same index to appear multiple times. Perhaps, it can avoid throwing error but that would still ignore other index and keep only 1 (without ordering specs).

@TheBaradhur
Copy link

@ajcvickers I'm actually really sad you removed that tag. I can't scaffold my model automatically using EFCore because of that and I need a long term solution to that index issue.

Any chance you could implement quickly @smitpatel suggestion for it to at least not throw and expection and just not create those problematic indexes?

Thanks!

@divega
Copy link
Contributor

divega commented May 25, 2018

@TheBaradhur the “consider-for-next-release” label is assigned temporarily for release planning. This is now assigned to the 2.2.0 milestone (at least for now), which has more significance.

@TheBaradhur
Copy link

@divega thank you for the update. But considering that 2.1 is not even officially released, I'm looking at months, if not a year, before this issue can be fixed. I will have to find other ways to scaffold my db until then.

@ErikEJ
Copy link
Contributor

ErikEJ commented May 26, 2018

It could be fixed in the Npgsql provider

@roji
Copy link
Member Author

roji commented May 26, 2018

@ErikEJ is right, I'll make a change in the Npgsql provider to make sure it does not throw in this case, at least as a workaround until the core issue is resolved.

roji added a commit to npgsql/efcore.pg that referenced this issue May 26, 2018
When scaffolding, we have an Npgsql-specific IndexMethod annotation to
represent PostgreSQL's index methods. Previously, we would always
output this annotation on indices, even when the method was the
default (btree); NpgsqlAnnotationCodeGenerator would elide it as by-
convention.

Although this is cleaner, it caused issues whenever two indices where
defined on the same column:
dotnet/efcore#11846

We now scaffold the annotation only for non-default index methods as a
workaround; the issue should now affect much less people.

Fixes #228
roji added a commit to npgsql/efcore.pg that referenced this issue May 26, 2018
When scaffolding, we have an Npgsql-specific IndexMethod annotation to
represent PostgreSQL's index methods. Previously, we would always
output this annotation on indices, even when the method was the
default (btree); NpgsqlAnnotationCodeGenerator would elide it as by-
convention.

Although this is cleaner, it caused issues whenever two indices where
defined on the same column:
dotnet/efcore#11846

We now scaffold the annotation only for non-default index methods as a
workaround; the issue should now affect much less people.

Fixes #228

(cherry picked from commit 38d2176)
@roji
Copy link
Member Author

roji commented May 26, 2018

@divega, @smitpatel, I've pushed a workaround which makes this issue go away for the majority of users, making this less urgent (I now include the IndexMethod annotation in the scaffolded database model only if it's the PostgreSQL non-default one). The issue is still there but shouldn't bother most people anymore.

@roji
Copy link
Member Author

roji commented May 26, 2018

However, EF Core is still incapable of scaffolding two indices on the same column (ascending and descending) which does seem to be a serious issue (and has nothing to do with PostgreSQL).

@ErikEJ
Copy link
Contributor

ErikEJ commented May 26, 2018

The value of DESC columns in indexes is debateable

@roji
Copy link
Member Author

roji commented May 26, 2018

@ErikEJ I think that depends on specific databases... I admit I'm not sure exactly what the impact is in PostgreSQL, but it seems like if it's supported in DDL EF Core should be able to render it...

@TheBaradhur
Copy link

@ErikEJ We use double indexes on same column with a tolower conditions in our PostgreSQL db, a few with DESC, in a lot of different tables. Especially the ones where we are dealing with 10th of thousands of rows in the table.

Having researched the internet about that double index issues in EFCore, I can tell you that it is not uncommon.

@divega
Copy link
Contributor

divega commented May 26, 2018

DESC on a column in an index is useful when queries have ORDER BY clauses on multiple columns with mixed order.

In SQL Server, very often tables are clustered (i.e. organized) on the PK, hence you can think of indexes on those tables as always containing an implicit reference to the PK column or columns. For example if on SQL Server you want to execute:

SELECT *
FROM Customers
ORDER BY CutomerID, Name DESC

Then an index on Name DESC can help.

I don’t know much about PostgreSQL, but the information I was able to find says it does not support index organized tables. That means that for DESC to be useful on an index on PostgreSQL, you need to have an index on more than one column, and the same or opposite mixed order on both the index and the queries.

AFAIK, having DESC on an index on a single column should never be necessary on PostgreSQL, as indexes can be used to return results ordered in the opposite direction.

Re tolower, are those using a function or computed column, or is tolower an attribute of the index?

Some interesting reads:

https://stackoverflow.com/questions/743858/sql-server-indexes-ascending-or-descending-what-difference-does-it-make

https://use-the-index-luke.com/sql/sorting-grouping/order-by-asc-desc-nulls-last

@roji
Copy link
Member Author

roji commented May 27, 2018

@divega an index on tolower() in PostgreSQL would be an expression index. There is no specific support for those in the Npgsql provider (see npgsql/efcore.pg#119) but they can be created with raw SQL.

To summarize, I think there are several legitimate reasons why someone would have multiple indices on the same column set; between expression indices, ASC/DESC, index methods (a concept specific to PostgreSQL) I think there's enough material. At the moment, when scaffolding EF Core will retain the last index present on the database model, and silently discard the others.

@divega
Copy link
Contributor

divega commented May 27, 2018

an index on tolower() in PostgreSQL would be an expression index

Makes sense. This is what I was expecting to hear 😄

there are several legitimate reasons why someone would have multiple indices on the same column set

Agreed. However I wonder if it would be better to change EF Core to separate the common case (index over a sequence of columns with implicit ASC on each of them, which we already support) from all other variations. EF Core would still reason about the column set for the simple scenario, but the index definition would become "custom"/provider-specific (and opaque to EF Core) for all other cases. Then we would never try to match a simple index against custom indexes based on the columns.

@ajcvickers
Copy link
Contributor

Closing this in favor of #4150

@ajcvickers ajcvickers reopened this Jun 4, 2018
@ajcvickers ajcvickers removed this from the 2.2.0 milestone Jun 4, 2018
@ajcvickers ajcvickers modified the milestones: 3.0.0, Backlog May 10, 2019
@yyjdelete
Copy link

yyjdelete commented Jun 24, 2020

It would be helpful after #17083 is fixed. And at least ignore the second index in VisitIndex instead of simply crash .

For something like

CREATE NONCLUSTERED INDEX [Z_Common] ON [Table]
([A],[B])
INCLUDE([C],[D])
//WHERE ([Z] = 1)



CREATE NONCLUSTERED INDEX [Z_0] ON [Table]
([A],[B])
INCLUDE([C],[D],[E],[F]) 
WHERE ([Z] = 0)

@AndriySvyryd
Copy link
Member

Named indexes are now supported, so this can be fixed properly

@lauxjpn
Copy link
Contributor

lauxjpn commented Oct 12, 2020

Got about the same issue reported for Pomelo:
PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1189 (comment)

This issues appears, when some facet of the index (in this case the prefix) is being represented internally by an annotation. The moment an annotation of the same type (in this case MySql:IndexPrefixLength) is being set for the second time for an object (here: index), the exception is being thrown.

Could it be related to this: dotnet/efcore#11846 ?

I can confirm, that the moment two indices are used for the same column, and both are using a prefix length, I can reproduce the exception:

CREATE TABLE `Test` (
  `TestId` int NOT NULL AUTO_INCREMENT,
  `LongString` varchar(12000) DEFAULT NULL,
  PRIMARY KEY (`TestId`),
  KEY `IX_LongString` (`LongString`(120)),
  KEY `IX_LongString2` (`LongString`(100))
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;

Basically, the `LongString`(120) and `LongString`(100) parts get translated into a MySql:IndexPrefixLength annotation respectively, that should be set only once per index. The indices are individually named, but get collapsed/keyed into one single index builder, that throws once the annotation gets set the second time (and also overwrites the first index name with the second one).

@roji
Copy link
Member Author

roji commented Sep 7, 2021

I've just checked this, and everything seems to be working just fine as of 6.0.0-preview.7 - #25917 adds a test for scaffolding two SQL Server indexes on the same column (with different fill factors). I've also checked the Npgsql provider, where ascending/descending is supported, and that both scaffolds successfully and gets applied back when creating a new database. Note that this does not mean that ascending/descending is supported (aside from PostgreSQL).

@lauxjpn hopefully you can give this a try to confirm that it works with the MySQL provider as well.

@roji roji assigned roji and unassigned bricelam Sep 7, 2021
@roji roji added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed poachable labels Sep 7, 2021
@smitpatel
Copy link
Contributor

Fixed by using named indexes in metadata.

@roji
Copy link
Member Author

roji commented Sep 7, 2021

Yep

@roji
Copy link
Member Author

roji commented Sep 7, 2021

Can maybe be considered a dup of #21089

@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Sep 8, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc2, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-scaffolding closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-2.2 punted-for-3.0 type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants