From 01d4ea90e6111396b7046bea67e385071c3409ae Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Tue, 26 Mar 2024 12:21:19 +0000 Subject: [PATCH 1/2] Apply fill factor when creating tables Fixes #33269 --- .../Migrations/MigrationsSqlGenerator.cs | 15 ++++---- .../SqlServerMigrationsSqlGenerator.cs | 20 +++-------- .../Migrations/MigrationsSqlServerTest.cs | 36 +++++++++++++++++++ 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs index b7cb97414d3..39687ef1836 100644 --- a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs +++ b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs @@ -241,8 +241,6 @@ protected virtual void Generate( PrimaryKeyConstraint(operation, model, builder); - KeyWithOptions(operation, builder); - if (terminate) { builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); @@ -269,8 +267,6 @@ protected virtual void Generate( UniqueConstraint(operation, model, builder); - KeyWithOptions(operation, builder); - builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); EndStatement(builder); } @@ -1622,6 +1618,8 @@ protected virtual void PrimaryKeyConstraint( builder.Append("(") .Append(ColumnList(operation.Columns)) .Append(")"); + + KeyTraits(operation, model, builder); } /// @@ -1669,6 +1667,8 @@ protected virtual void UniqueConstraint( builder.Append("(") .Append(ColumnList(operation.Columns)) .Append(")"); + + KeyTraits(operation, model, builder); } /// @@ -1717,12 +1717,13 @@ protected virtual void CheckConstraint( } /// - /// Generates a SQL fragment for extra with options of a key from a - /// or . + /// Generates a SQL fragment for traits of an primary key or alternate key from a , + /// , or . /// /// The operation. + /// The target model which may be if the operations exist without a model. /// The command builder to use to add the SQL fragment. - protected virtual void KeyWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) + protected virtual void KeyTraits(MigrationOperation operation, IModel? model, MigrationCommandListBuilder builder) { } diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index d96997a0d76..0ff8a6054d4 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1775,26 +1775,14 @@ protected virtual void Transfer( } } - /// - /// Generates a SQL fragment for extra with options of a key from a - /// , or . - /// - /// The operation. - /// The command builder to use to add the SQL fragment. - protected override void KeyWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) + /// + protected override void KeyTraits(MigrationOperation operation, IModel? model, MigrationCommandListBuilder builder) { - var options = new List(); - if (operation[SqlServerAnnotationNames.FillFactor] is int fillFactor) - { - options.Add("FILLFACTOR = " + fillFactor); - } - - if (options.Count > 0) { builder - .Append(" WITH (") - .Append(string.Join(", ", options)) + .Append(" WITH (FILLFACTOR = ") + .Append(fillFactor.ToString()) .Append(")"); } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs index d31a5e10ee4..156014abca2 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs @@ -354,6 +354,42 @@ PERIOD FOR SYSTEM_TIME([PeriodStart], [PeriodEnd]) """); } + [ConditionalFact] + public virtual async Task Create_table_with_fill_factor() + { + await Test( + _ => { }, + builder => + { + builder.Entity("People").Property("TheKey"); + builder.Entity("People").Property("TheAlternateKey"); + builder.Entity("People").HasKey("TheKey").HasFillFactor(81); + builder.Entity("People").HasAlternateKey("TheAlternateKey").HasFillFactor(82); + }, + model => + { + var table = Assert.Single(model.Tables); + + var primaryKey = table.PrimaryKey; + Assert.NotNull(primaryKey); + Assert.Equal(81, primaryKey[SqlServerAnnotationNames.FillFactor]); + + var uniqueConstraint = table.UniqueConstraints.FirstOrDefault(); + Assert.NotNull(uniqueConstraint); + Assert.Equal(82, uniqueConstraint[SqlServerAnnotationNames.FillFactor]); + }); + + AssertSql( + """ +CREATE TABLE [People] ( + [TheKey] int NOT NULL IDENTITY, + [TheAlternateKey] uniqueidentifier NOT NULL, + CONSTRAINT [PK_People] PRIMARY KEY ([TheKey]) WITH (FILLFACTOR = 81), + CONSTRAINT [AK_People_TheAlternateKey] UNIQUE ([TheAlternateKey]) WITH (FILLFACTOR = 82) +); +"""); + } + public override async Task Drop_table() { await base.Drop_table(); From bd98dd5c41d631466f708c6c0a92090773b63c35 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Wed, 27 Mar 2024 11:33:20 +0000 Subject: [PATCH 2/2] Updated to remove KeyTraits and use IndexOptions --- .../Migrations/MigrationsSqlGenerator.cs | 22 ++----- .../SqlServerMigrationsSqlGenerator.cs | 60 ++++++++----------- 2 files changed, 31 insertions(+), 51 deletions(-) diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs index 39687ef1836..9d4620b6ce8 100644 --- a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs +++ b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs @@ -1619,7 +1619,7 @@ protected virtual void PrimaryKeyConstraint( .Append(ColumnList(operation.Columns)) .Append(")"); - KeyTraits(operation, model, builder); + IndexOptions(operation, model, builder); } /// @@ -1668,7 +1668,7 @@ protected virtual void UniqueConstraint( .Append(ColumnList(operation.Columns)) .Append(")"); - KeyTraits(operation, model, builder); + IndexOptions(operation, model, builder); } /// @@ -1716,17 +1716,6 @@ protected virtual void CheckConstraint( .Append(")"); } - /// - /// Generates a SQL fragment for traits of an primary key or alternate key from a , - /// , or . - /// - /// The operation. - /// The target model which may be if the operations exist without a model. - /// The command builder to use to add the SQL fragment. - protected virtual void KeyTraits(MigrationOperation operation, IModel? model, MigrationCommandListBuilder builder) - { - } - /// /// Generates a SQL fragment for traits of an index from a , /// , or . @@ -1768,13 +1757,14 @@ protected virtual void GenerateIndexColumnList(CreateIndexOperation operation, I /// The operation. /// The target model which may be if the operations exist without a model. /// The command builder to use to add the SQL fragment. - protected virtual void IndexOptions(CreateIndexOperation operation, IModel? model, MigrationCommandListBuilder builder) + protected virtual void IndexOptions(MigrationOperation operation, IModel? model, MigrationCommandListBuilder builder) { - if (!string.IsNullOrEmpty(operation.Filter)) + if (operation is CreateIndexOperation createIndexOperation + && !string.IsNullOrEmpty(createIndexOperation.Filter)) { builder .Append(" WHERE ") - .Append(operation.Filter); + .Append(createIndexOperation.Filter); } } diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 0ff8a6054d4..dbff4b7a514 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections; using System.Globalization; using System.Text; using Microsoft.EntityFrameworkCore.SqlServer.Internal; @@ -1775,18 +1774,6 @@ protected virtual void Transfer( } } - /// - protected override void KeyTraits(MigrationOperation operation, IModel? model, MigrationCommandListBuilder builder) - { - if (operation[SqlServerAnnotationNames.FillFactor] is int fillFactor) - { - builder - .Append(" WITH (FILLFACTOR = ") - .Append(fillFactor.ToString()) - .Append(")"); - } - } - /// /// Generates a SQL fragment for traits of an index from a , /// , or . @@ -1808,7 +1795,7 @@ protected override void IndexTraits(MigrationOperation operation, IModel? model, /// The operation. /// The target model which may be if the operations exist without a model. /// The command builder to use to add the SQL fragment. - protected override void IndexOptions(CreateIndexOperation operation, IModel? model, MigrationCommandListBuilder builder) + protected override void IndexOptions(MigrationOperation operation, IModel? model, MigrationCommandListBuilder builder) { if (operation[SqlServerAnnotationNames.Include] is IReadOnlyList includeColumns && includeColumns.Count > 0) @@ -1827,37 +1814,40 @@ protected override void IndexOptions(CreateIndexOperation operation, IModel? mod builder.Append(")"); } - if (!string.IsNullOrEmpty(operation.Filter)) - { - builder - .Append(" WHERE ") - .Append(operation.Filter); - } - else if (UseLegacyIndexFilters(operation, model)) + if (operation is CreateIndexOperation createIndexOperation) { - var table = model?.GetRelationalModel().FindTable(operation.Table, operation.Schema); - var nullableColumns = operation.Columns - .Where(c => table?.FindColumn(c)?.IsNullable != false) - .ToList(); - - builder.Append(" WHERE "); - for (var i = 0; i < nullableColumns.Count; i++) + if (!string.IsNullOrEmpty(createIndexOperation.Filter)) + { + builder + .Append(" WHERE ") + .Append(createIndexOperation.Filter); + } + else if (UseLegacyIndexFilters(createIndexOperation, model)) { - if (i != 0) + var table = model?.GetRelationalModel().FindTable(createIndexOperation.Table, createIndexOperation.Schema); + var nullableColumns = createIndexOperation.Columns + .Where(c => table?.FindColumn(c)?.IsNullable != false) + .ToList(); + + builder.Append(" WHERE "); + for (var i = 0; i < nullableColumns.Count; i++) { - builder.Append(" AND "); - } + if (i != 0) + { + builder.Append(" AND "); + } - builder - .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(nullableColumns[i])) - .Append(" IS NOT NULL"); + builder + .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(nullableColumns[i])) + .Append(" IS NOT NULL"); + } } } IndexWithOptions(operation, builder); } - private static void IndexWithOptions(CreateIndexOperation operation, MigrationCommandListBuilder builder) + private static void IndexWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) { var options = new List();