Skip to content

Scaffold index operators and included (covered) index columns #709

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.

Sign up for GitHub

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

Merged
roji merged 2 commits into from
Nov 17, 2018
Merged

Scaffold index operators and included (covered) index columns #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e3ed52
Scaffold index operators
roji Nov 17, 2018
79f86da
Scaffold included (covered) index columns
roji Nov 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions src/EFCore.PG/Scaffolding/Internal/NpgsqlDatabaseModelFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,15 +412,25 @@ static void GetIndexes(
[NotNull] List<uint> constraintIndexes,
[NotNull] IDiagnosticsLogger<DbLoggerCategory.Scaffolding> logger)
{
// Load the pg_opclass table (https://www.postgresql.org/docs/current/catalog-pg-opclass.html),
// which is referenced by the indices we'll load below
var opClasses = new Dictionary<uint, (string Name, bool IsDefault)>();
using (var command = new NpgsqlCommand("SELECT oid, opcname, opcdefault FROM pg_opclass", connection))
using (var reader = command.ExecuteReader())
foreach (var opClass in reader.Cast<DbDataRecord>())
opClasses[opClass.GetValueOrDefault<uint>("oid")] = (opClass.GetValueOrDefault<string>("opcname"), opClass.GetValueOrDefault<bool>("opcdefault"));

var commandText = $@"
SELECT
idxcls.oid AS idx_oid,
nspname,
cls.relname AS cls_relname,
idxcls.relname AS idx_relname,
indisunique,
{(connection.PostgreSqlVersion >= new Version(11, 0) ? "indnkeyatts" : "indnatts AS indnkeyatts")},
indkey,
amname,
indclass,
CASE
WHEN indexprs IS NULL THEN NULL
ELSE pg_get_expr(indexprs, cls.oid)
Expand Down Expand Up @@ -469,7 +479,10 @@ NOT indisprimary AND
IsUnique = record.GetValueOrDefault<bool>("indisunique")
};

var numKeyColumns = record.GetValueOrDefault<short>("indnkeyatts");
var columnIndices = record.GetValueOrDefault<short[]>("indkey");
var tableColumns = (List<DatabaseColumn>)table.Columns;

if (columnIndices.Any(i => i == 0))
{
// Expression index, not supported
Expand All @@ -484,19 +497,36 @@ NOT indisprimary AND
*/
}

var columns = (List<DatabaseColumn>)table.Columns;
foreach (var i in columnIndices)
// Key columns come before non-key (included) columns, process them first
foreach (var i in columnIndices.Take(numKeyColumns))
{
if (columns[i - 1] is DatabaseColumn indexColumn)
index.Columns.Add(indexColumn);

if (tableColumns[i - 1] is DatabaseColumn indexKeyColumn)
index.Columns.Add(indexKeyColumn);
else
{
logger.UnsupportedColumnIndexSkippedWarning(index.Name, DisplayName(tableSchema, tableName));
goto IndexEnd;
}
}

// Now go over non-key (included columns) if any are present
if (columnIndices.Length > numKeyColumns)
{
var nonKeyColumns = new List<string>();
foreach (var i in columnIndices.Skip(numKeyColumns))
{
if (tableColumns[i - 1] is DatabaseColumn indexKeyColumn)
nonKeyColumns.Add(indexKeyColumn.Name);
else
{
logger.UnsupportedColumnIndexSkippedWarning(index.Name, DisplayName(tableSchema, tableName));
goto IndexEnd;
}
}

index[NpgsqlAnnotationNames.IndexInclude] = nonKeyColumns.ToArray();
}

if (record.GetValueOrDefault<string>("pred") is string predicate)
index.Filter = predicate;

Expand All @@ -508,6 +538,14 @@ NOT indisprimary AND
if (record.GetValueOrDefault<string>("amname") is string indexMethod && indexMethod != "btree")
index[NpgsqlAnnotationNames.IndexMethod] = indexMethod;

// Handle index operator classes, which we pre-loaded
var opClassNames = record
.GetValueOrDefault<uint[]>("indclass")
.Select(oid => opClasses.TryGetValue(oid, out var opc) && !opc.IsDefault ? opc.Name : null)
.ToArray();
if (opClassNames.Any(op => op != null))
index[NpgsqlAnnotationNames.IndexOperators] = opClassNames;

table.Indexes.Add(index);

IndexEnd: ;
Expand Down
52 changes: 52 additions & 0 deletions test/EFCore.PG.FunctionalTests/Scaffolding/NpgsqlDatabaseModelFactoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,6 +1532,58 @@ public void Index_method()
@"DROP TABLE ""IndexMethod""");
}

[Fact]
public void Index_operators()
{
Test(
@"
CREATE TABLE ""IndexOperators"" (a text, b text);
CREATE INDEX ix_with ON ""IndexOperators"" (a, b varchar_pattern_ops);
CREATE INDEX ix_without ON ""IndexOperators"" (a, b);",
Enumerable.Empty<string>(),
Enumerable.Empty<string>(),
dbModel =>
{
var table = dbModel.Tables.Single();

var indexWith = table.Indexes.Single(i => i.Name == "ix_with");
Assert.Equal(new[] { null, "varchar_pattern_ops" }, indexWith.FindAnnotation(NpgsqlAnnotationNames.IndexOperators).Value);

var indexWithout = table.Indexes.Single(i => i.Name == "ix_without");
Assert.Null(indexWithout.FindAnnotation(NpgsqlAnnotationNames.IndexOperators));
},
@"DROP TABLE ""IndexOperators""");
}

[Fact]
public void Index_covering()
{
if (Fixture.TestStore.GetPostgresVersion() < new Version(11, 0))
Copy link
Copy Markdown
Contributor

@austindrenski austindrenski Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, but should we at some point start testing against multiple release versions of PostgreSQL?

Might be something to consider in a few weeks, after 2.2 and the move to Azure Pipelines.

Copy link
Copy Markdown
Member Author

@roji roji Nov 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A long, long time ago it actually worked this way. The problems were a much longer build time (multiplied by the number of versions supported), and significantly more complexity in maintenance of the tests themselves and/or the CI platform (this may be partially gone with Azure, or not).

As a general rule, I'm not sure we've seen many version-related issues that weren't reported and fixed rather quickly. So while testing all versions is theoretically better, I'm simply not sure the added value is worth the downsides... Am definitely open to discussing this though - maybe open a new issue for discussing?

return;

Test(
@"
CREATE TABLE ""IndexCovering"" (a text, b text, c text);
CREATE INDEX ix_with ON ""IndexCovering"" (a) INCLUDE (b, c);
CREATE INDEX ix_without ON ""IndexCovering"" (a, b, c);",
Enumerable.Empty<string>(),
Enumerable.Empty<string>(),
dbModel =>
{
var table = dbModel.Tables.Single();

var indexWith = table.Indexes.Single(i => i.Name == "ix_with");
Assert.Equal("a", indexWith.Columns.Single().Name);
Assert.Equal(new[] { "b", "c" }, indexWith.FindAnnotation(NpgsqlAnnotationNames.IndexInclude).Value);

var indexWithout = table.Indexes.Single(i => i.Name == "ix_without");
Assert.Equal(new[] { "a", "b", "c" }, indexWithout.Columns.Select(i => i.Name).ToArray());
Assert.Null(indexWithout.FindAnnotation(NpgsqlAnnotationNames.IndexInclude));

},
@"DROP TABLE ""IndexCovering""");
}

[Fact]
public void Comments()
{
Expand Down
20 changes: 20 additions & 0 deletions test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,26 @@ private static DbCommand CreateCommand(
return command;
}

public Version GetPostgresVersion()
{
var opened = false;
if (Connection.State == ConnectionState.Closed)
{
Connection.Open();
opened = true;
}

try
{
return ((NpgsqlConnection)Connection).PostgreSqlVersion;
Comment thread
roji marked this conversation as resolved.
}
finally
{
if (opened)
Connection.Close();
}
}

public override void Dispose()
{
base.Dispose();
Expand Down