From 1eef30710d3b7560faab55993748c96132ba1b67 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 12 Jul 2016 17:34:11 +0200 Subject: [PATCH 1/4] U4-8522 cmsPropertyData needs unique constraint indexes and index refactoring --- .../Syntax/Delete/Index/DeleteIndexBuilder.cs | 5 +- .../Index/IDeleteIndexOnColumnSyntax.cs | 7 +- .../UpdateUniqueIndexOnCmsPropertyData.cs | 114 ++++++++++++++++++ src/Umbraco.Core/Umbraco.Core.csproj | 1 + 4 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs diff --git a/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/DeleteIndexBuilder.cs b/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/DeleteIndexBuilder.cs index 72250877db..93661c9ece 100644 --- a/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/DeleteIndexBuilder.cs +++ b/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/DeleteIndexBuilder.cs @@ -1,4 +1,5 @@ -using Umbraco.Core.Persistence.DatabaseModelDefinitions; +using System; +using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Migrations.Syntax.Delete.Expressions; namespace Umbraco.Core.Persistence.Migrations.Syntax.Delete.Index @@ -17,12 +18,14 @@ namespace Umbraco.Core.Persistence.Migrations.Syntax.Delete.Index return this; } + [Obsolete("I don't think this would ever be used when dropping an index, see DeleteIndexExpression.ToString")] public void OnColumn(string columnName) { var column = new IndexColumnDefinition { Name = columnName }; Expression.Index.Columns.Add(column); } + [Obsolete("I don't think this would ever be used when dropping an index, see DeleteIndexExpression.ToString")] public void OnColumns(params string[] columnNames) { foreach (string columnName in columnNames) diff --git a/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/IDeleteIndexOnColumnSyntax.cs b/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/IDeleteIndexOnColumnSyntax.cs index f2f4280f23..fcf5038a86 100644 --- a/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/IDeleteIndexOnColumnSyntax.cs +++ b/src/Umbraco.Core/Persistence/Migrations/Syntax/Delete/Index/IDeleteIndexOnColumnSyntax.cs @@ -1,8 +1,13 @@ -namespace Umbraco.Core.Persistence.Migrations.Syntax.Delete.Index +using System; + +namespace Umbraco.Core.Persistence.Migrations.Syntax.Delete.Index { public interface IDeleteIndexOnColumnSyntax : IFluentSyntax { + [Obsolete("I don't think this would ever be used when dropping an index, see DeleteIndexExpression.ToString")] void OnColumn(string columnName); + + [Obsolete("I don't think this would ever be used when dropping an index, see DeleteIndexExpression.ToString")] void OnColumns(params string[] columnNames); } } \ No newline at end of file diff --git a/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs b/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs new file mode 100644 index 0000000000..be576ebbe5 --- /dev/null +++ b/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs @@ -0,0 +1,114 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using Umbraco.Core.Configuration; +using Umbraco.Core.Logging; +using Umbraco.Core.Models.Rdbms; +using Umbraco.Core.Persistence.SqlSyntax; + +namespace Umbraco.Core.Persistence.Migrations.Upgrades.TargetVersionSevenFiveZero +{ + /// + /// See: http://issues.umbraco.org/issue/U4-8522 + /// + [Migration("7.5.0", 2, GlobalSettings.UmbracoMigrationName)] + public class UpdateUniqueIndexOnCmsPropertyData : MigrationBase + { + public UpdateUniqueIndexOnCmsPropertyData(ISqlSyntaxProvider sqlSyntax, ILogger logger) + : base(sqlSyntax, logger) + { + } + + public override void Up() + { + //Clear all stylesheet data if the tables exist + //tuple = tablename, indexname, columnname, unique + var indexes = SqlSyntax.GetDefinedIndexes(Context.Database).ToArray(); + var found = indexes.FirstOrDefault( + x => x.Item1.InvariantEquals("cmsPropertyData") + && x.Item2.InvariantEquals("IX_cmsPropertyData_1") + //we're searching for the old index which is not unique + && x.Item4 == false); + + if (found != null) + { + ////NOTE: WE cannot execute this SQL inside of an Execute block because even though we are removing duplicates, + ////we cannot drop/add an index until that trans is completed, so we need to do this eagerly. + + //In order to apply this unique index, we must ensure that there is no duplicate data. + // so we need to query for this + Execute.Code(database => + { + const string sql = @"SELECT mt.id, mt.contentNodeId, mt.versionId, mt.propertytypeId FROM cmsPropertyData mt +INNER JOIN ( + SELECT contentNodeId, versionId, propertytypeId + FROM cmsPropertyData + GROUP BY contentNodeId, versionId, propertytypeId + HAVING COUNT(*) >1 +) T +ON mt.contentNodeId= T.contentNodeId +and mt.versionId= T.versionId +and mt.propertytypeId= T.propertytypeId +ORDER BY mt.id"; + + var duplicates = database.Query(sql); + var currDuplicates = new List>(); + foreach (var duplicate in duplicates) + { + //check if the current duplicates batch is changing as we iterate + if (currDuplicates.Count > 0 + && currDuplicates[0].Item2 == duplicate.contentNodeId + && currDuplicates[0].Item3 == duplicate.versionId + && currDuplicates[0].Item4 == duplicate.propertytypeId) + { + //this is still a duplicate so add it + AddDuplicate(duplicate, currDuplicates); + } + else + { + //the batch is changing so perform the deletions except for last + for (var index = 0; index < (currDuplicates.Count - 1); index++) + { + var currDuplicate = currDuplicates[index]; + database.Delete(currDuplicate.Item1); + } + currDuplicates.Clear(); + //add the next batch + AddDuplicate(duplicate, currDuplicates); + } + } + + //now we need to process the last batch + for (var index = 0; index < (currDuplicates.Count - 1); index++) + { + var currDuplicate = currDuplicates[index]; + var result = database.Delete(currDuplicate.Item1); + Debug.Assert(result == 1); + } + currDuplicates.Clear(); + + return string.Empty; + }); + + //we need to re create this index + Delete.Index("IX_cmsPropertyData_1").OnTable("cmsPropertyData"); + Create.Index("IX_cmsPropertyData_1").OnTable("cmsPropertyData") + .OnColumn("contentNodeId").Ascending() + .OnColumn("versionId").Ascending() + .OnColumn("propertytypeid").Ascending() + .WithOptions().NonClustered() + .WithOptions().Unique(); + } + } + + private void AddDuplicate(dynamic duplicate, List> list) + { + list.Add(new Tuple(duplicate.id, duplicate.contentNodeId, duplicate.versionId, duplicate.propertytypeId)); + } + + public override void Down() + { + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 749502b310..22aa68ab7a 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -433,6 +433,7 @@ + From bd306295c2c65209f91d3498c9a00dc85a7387c1 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 12 Jul 2016 19:39:22 +0200 Subject: [PATCH 2/4] Ensures the correct index is installed by default, fixes a test that didn't take this index into account --- src/Umbraco.Core/Models/Rdbms/PropertyDataDto.cs | 2 +- src/Umbraco.Tests/Migrations/MigrationIssuesTests.cs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Models/Rdbms/PropertyDataDto.cs b/src/Umbraco.Core/Models/Rdbms/PropertyDataDto.cs index 380eaa3034..c4cd28f6e0 100644 --- a/src/Umbraco.Core/Models/Rdbms/PropertyDataDto.cs +++ b/src/Umbraco.Core/Models/Rdbms/PropertyDataDto.cs @@ -16,7 +16,7 @@ namespace Umbraco.Core.Models.Rdbms [Column("contentNodeId")] [ForeignKey(typeof(NodeDto))] - [Index(IndexTypes.NonClustered, Name = "IX_cmsPropertyData_1")] + [Index(IndexTypes.UniqueNonClustered, Name = "IX_cmsPropertyData_1", ForColumns = "contentNodeId,versionId,propertytypeid")] public int NodeId { get; set; } [Column("versionId")] diff --git a/src/Umbraco.Tests/Migrations/MigrationIssuesTests.cs b/src/Umbraco.Tests/Migrations/MigrationIssuesTests.cs index 872495dd7f..7d1a97f19d 100644 --- a/src/Umbraco.Tests/Migrations/MigrationIssuesTests.cs +++ b/src/Umbraco.Tests/Migrations/MigrationIssuesTests.cs @@ -80,14 +80,16 @@ namespace Umbraco.Tests.Migrations { NodeId = n.NodeId, PropertyTypeId = pt.Id, - Text = "text" + Text = "text", + VersionId = Guid.NewGuid() }; DatabaseContext.Database.Insert(data); data = new PropertyDataDto { NodeId = n.NodeId, PropertyTypeId = pt.Id, - Text = "" + Text = "", + VersionId = Guid.NewGuid() }; DatabaseContext.Database.Insert(data); From e7b26ad949eed90644c1749575ee84882110b58b Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 14 Jul 2016 17:51:18 +0200 Subject: [PATCH 3/4] updates migratoin to run a single delete statement during execution --- .../UpdateUniqueIndexOnCmsPropertyData.cs | 63 +------------------ 1 file changed, 1 insertion(+), 62 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs b/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs index be576ebbe5..0896af42e7 100644 --- a/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs +++ b/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs @@ -33,63 +33,7 @@ namespace Umbraco.Core.Persistence.Migrations.Upgrades.TargetVersionSevenFiveZer if (found != null) { - ////NOTE: WE cannot execute this SQL inside of an Execute block because even though we are removing duplicates, - ////we cannot drop/add an index until that trans is completed, so we need to do this eagerly. - - //In order to apply this unique index, we must ensure that there is no duplicate data. - // so we need to query for this - Execute.Code(database => - { - const string sql = @"SELECT mt.id, mt.contentNodeId, mt.versionId, mt.propertytypeId FROM cmsPropertyData mt -INNER JOIN ( - SELECT contentNodeId, versionId, propertytypeId - FROM cmsPropertyData - GROUP BY contentNodeId, versionId, propertytypeId - HAVING COUNT(*) >1 -) T -ON mt.contentNodeId= T.contentNodeId -and mt.versionId= T.versionId -and mt.propertytypeId= T.propertytypeId -ORDER BY mt.id"; - - var duplicates = database.Query(sql); - var currDuplicates = new List>(); - foreach (var duplicate in duplicates) - { - //check if the current duplicates batch is changing as we iterate - if (currDuplicates.Count > 0 - && currDuplicates[0].Item2 == duplicate.contentNodeId - && currDuplicates[0].Item3 == duplicate.versionId - && currDuplicates[0].Item4 == duplicate.propertytypeId) - { - //this is still a duplicate so add it - AddDuplicate(duplicate, currDuplicates); - } - else - { - //the batch is changing so perform the deletions except for last - for (var index = 0; index < (currDuplicates.Count - 1); index++) - { - var currDuplicate = currDuplicates[index]; - database.Delete(currDuplicate.Item1); - } - currDuplicates.Clear(); - //add the next batch - AddDuplicate(duplicate, currDuplicates); - } - } - - //now we need to process the last batch - for (var index = 0; index < (currDuplicates.Count - 1); index++) - { - var currDuplicate = currDuplicates[index]; - var result = database.Delete(currDuplicate.Item1); - Debug.Assert(result == 1); - } - currDuplicates.Clear(); - - return string.Empty; - }); + Execute.Sql("DELETE FROM cmsPropertyData WHERE id NOT IN (SELECT MIN(id) FROM cmsPropertyData GROUP BY contentNodeId, versionId, propertytypeid HAVING MIN(id) IS NOT NULL)"); //we need to re create this index Delete.Index("IX_cmsPropertyData_1").OnTable("cmsPropertyData"); @@ -102,11 +46,6 @@ ORDER BY mt.id"; } } - private void AddDuplicate(dynamic duplicate, List> list) - { - list.Add(new Tuple(duplicate.id, duplicate.contentNodeId, duplicate.versionId, duplicate.propertytypeId)); - } - public override void Down() { } From 52e2cbab9064b360342084037228c2ae3b217786 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 14 Jul 2016 18:00:43 +0200 Subject: [PATCH 4/4] updates delete query to work with MySql --- .../UpdateUniqueIndexOnCmsPropertyData.cs | 19 ++++++++++++++++++- .../SqlSyntax/SqlSyntaxProviderExtensions.cs | 15 +++++++++++++-- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs b/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs index 0896af42e7..4bae9c80e4 100644 --- a/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs +++ b/src/Umbraco.Core/Persistence/Migrations/Upgrades/TargetVersionSevenFiveZero/UpdateUniqueIndexOnCmsPropertyData.cs @@ -33,7 +33,24 @@ namespace Umbraco.Core.Persistence.Migrations.Upgrades.TargetVersionSevenFiveZer if (found != null) { - Execute.Sql("DELETE FROM cmsPropertyData WHERE id NOT IN (SELECT MIN(id) FROM cmsPropertyData GROUP BY contentNodeId, versionId, propertytypeid HAVING MIN(id) IS NOT NULL)"); + //Check for MySQL + if (Context.CurrentDatabaseProvider == DatabaseProviders.MySql) + { + //Use the special double nested sub query for MySQL since that is the only + //way delete sub queries works + SqlSyntax.GetDeleteSubquery( + "cmsPropertyData", + "id", + new Sql("SELECT MIN(id) FROM cmsPropertyData GROUP BY contentNodeId, versionId, propertytypeid HAVING MIN(id) IS NOT NULL"), + WhereInType.NotIn); + } + else + { + //NOTE: Even though the above will work for MSSQL, we are not going to execute the + // nested delete sub query logic since it will be slower and there could be a ton of property + // data here so needs to be as fast as possible. + Execute.Sql("DELETE FROM cmsPropertyData WHERE id NOT IN (SELECT MIN(id) FROM cmsPropertyData GROUP BY contentNodeId, versionId, propertytypeid HAVING MIN(id) IS NOT NULL)"); + } //we need to re create this index Delete.Index("IX_cmsPropertyData_1").OnTable("cmsPropertyData"); diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderExtensions.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderExtensions.cs index af18d9c3d9..1231765f20 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderExtensions.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderExtensions.cs @@ -22,12 +22,23 @@ /// /// See: http://issues.umbraco.org/issue/U4-3876 /// - public static Sql GetDeleteSubquery(this ISqlSyntaxProvider sqlProvider, string tableName, string columnName, Sql subQuery) + public static Sql GetDeleteSubquery(this ISqlSyntaxProvider sqlProvider, string tableName, string columnName, Sql subQuery, WhereInType whereInType = WhereInType.In) { - return new Sql(string.Format(@"DELETE FROM {0} WHERE {1} IN (SELECT {1} FROM ({2}) x)", + + return + new Sql(string.Format( + whereInType == WhereInType.In + ? @"DELETE FROM {0} WHERE {1} IN (SELECT {1} FROM ({2}) x)" + : @"DELETE FROM {0} WHERE {1} NOT IN (SELECT {1} FROM ({2}) x)", sqlProvider.GetQuotedTableName(tableName), sqlProvider.GetQuotedColumnName(columnName), subQuery.SQL), subQuery.Arguments); } } + + internal enum WhereInType + { + In, + NotIn + } } \ No newline at end of file