From 8fe0c3ca504e0e0a7f9529a5b3f05e3eb64217a4 Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Fri, 26 Jul 2024 18:43:16 -0700 Subject: [PATCH 01/10] Disabling EXISTS --- .../Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 13ec35c98d..5078588aa3 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -1229,7 +1229,7 @@ private bool CheckAppendWithJoin() } else { - return false; + return true; } } From eea073a9e8f850a6dc25d80976a3a1158e8c7e55 Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Sat, 27 Jul 2024 16:05:53 -0700 Subject: [PATCH 02/10] Comment --- .../QueryGenerators/SqlQueryGenerator.cs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 5078588aa3..5713bde369 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -514,7 +514,7 @@ private void HandleTableKindNormal(SearchParamTableExpression searchParamTableEx .Append(" AND ").Append(VLatest.Resource.ResourceSurrogateId, null).Append(" = ").AppendLine("Sid2"); } - if (CheckAppendWithJoin() + if (UseAppendWithJoin() && searchParamTableExpression.ChainLevel == 0 && !IsInSortMode(context)) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression); @@ -524,7 +524,7 @@ private void HandleTableKindNormal(SearchParamTableExpression searchParamTableEx { AppendHistoryClause(delimited, context.ResourceVersionTypes, searchParamTableExpression); - if (searchParamTableExpression.ChainLevel == 0 && !IsInSortMode(context) && !CheckAppendWithJoin()) + if (searchParamTableExpression.ChainLevel == 0 && !IsInSortMode(context) && !UseAppendWithJoin()) { // if chainLevel > 0 or if in sort mode or if we need to simplify the query, the intersection is already handled in a JOIN AppendIntersectionWithPredecessor(delimited, searchParamTableExpression); @@ -723,7 +723,7 @@ private void HandleTableKindChain( } // since we are in chain table expression, we know the Table is the ReferenceSearchParam table - else if (CheckAppendWithJoin()) + else if (UseAppendWithJoin()) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression, chainedExpression.Reversed ? referenceTargetResourceTableAlias : referenceSourceTableAlias); } @@ -746,7 +746,7 @@ private void HandleTableKindChain( .Append(string.Join(", ", chainedExpression.TargetResourceTypes.Select(x => Parameters.AddParameter(VLatest.ReferenceSearchParam.ReferenceResourceTypeId, Model.GetResourceTypeId(x), true)))) .Append(")"); - if (searchParamTableExpression.ChainLevel == 1 && !CheckAppendWithJoin()) + if (searchParamTableExpression.ChainLevel == 1 && !UseAppendWithJoin()) { // if > 1, the intersection is handled by the JOIN AppendIntersectionWithPredecessor(delimited, searchParamTableExpression, chainedExpression.Reversed ? referenceTargetResourceTableAlias : referenceSourceTableAlias); @@ -1054,7 +1054,7 @@ private void HandleTableKindSort(SearchParamTableExpression searchParamTableExpr .Append(sortContext.SortColumnName, null).AppendLine(" AS SortValue") .Append("FROM ").AppendLine(searchParamTableExpression.QueryGenerator.Table); - if (CheckAppendWithJoin()) + if (UseAppendWithJoin()) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression); } @@ -1081,7 +1081,7 @@ private void HandleTableKindSort(SearchParamTableExpression searchParamTableExpr StringBuilder.Append(" OR ").Append(sortContext.SortColumnName, null).Append(" ").Append(sortOperand).Append(" ").Append(Parameters.AddParameter(sortContext.SortColumnName, sortContext.SortValue, includeInHash: false)).AppendLine(")"); } - if (!CheckAppendWithJoin()) + if (!UseAppendWithJoin()) { AppendIntersectionWithPredecessor(delimited, searchParamTableExpression); } @@ -1103,7 +1103,7 @@ private void HandleTableKindSortWithFilter(SearchParamTableExpression searchPara .Append(sortContext.SortColumnName, null).AppendLine(" AS SortValue") .Append("FROM ").AppendLine(searchParamTableExpression.QueryGenerator.Table); - if (CheckAppendWithJoin()) + if (UseAppendWithJoin()) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression); } @@ -1130,7 +1130,7 @@ private void HandleTableKindSortWithFilter(SearchParamTableExpression searchPara StringBuilder.Append(" OR ").Append(sortContext.SortColumnName, null).Append(" ").Append(sortOperand).Append(" ").Append(Parameters.AddParameter(sortContext.SortColumnName, sortContext.SortValue, includeInHash: false)).AppendLine(")"); } - if (!CheckAppendWithJoin()) + if (!UseAppendWithJoin()) { AppendIntersectionWithPredecessor(delimited, searchParamTableExpression); } @@ -1216,12 +1216,12 @@ private void AppendNewTableExpression(IndentedStringBuilder sb, SearchParamTable sb.Append(")"); } - private bool CheckAppendWithJoin() + private bool UseAppendWithJoin() { // if either: // 1. the number of table expressions is greater than the limit indicating a complex query // 2. the previous query generator failed to generate a query - // then we will use the EXISTS clause instead of the inner join + // then we will NOT use the EXISTS clause instead of the inner join if (_rootExpression.SearchParamTableExpressions.Count > maxTableExpressionCountLimitForExists || previousSqlQueryGeneratorFailure) { @@ -1229,6 +1229,10 @@ private bool CheckAppendWithJoin() } else { + // Firstly, we were still using JOINs for filtering conditions for queries with include clauses. + // This does not make sense because filtering SQL should not be dependent on exiestence of include. + // It also appeared that SQL Server compiles plans for JOINs 5x faster than for EXISTSs. + // Though EXISTS plans should be more "precise" I am disabling them in favor of fast compilation times. return true; } } From 4cd13e5ec1890150e33a2c67b884dcd71f46c76d Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Sat, 27 Jul 2024 16:08:05 -0700 Subject: [PATCH 03/10] spelling --- .../Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 5713bde369..777269260e 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -1230,7 +1230,7 @@ private bool UseAppendWithJoin() else { // Firstly, we were still using JOINs for filtering conditions for queries with include clauses. - // This does not make sense because filtering SQL should not be dependent on exiestence of include. + // This does not make sense because filtering SQL should not be dependent on existence of include. // It also appeared that SQL Server compiles plans for JOINs 5x faster than for EXISTSs. // Though EXISTS plans should be more "precise" I am disabling them in favor of fast compilation times. return true; From 63c0a4d4b24ae8617ef631bff13c192ea9e43dc2 Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Sun, 28 Jul 2024 09:31:17 -0700 Subject: [PATCH 04/10] Removed not used parameter currentSearchParametersHash --- .../Visitors/QueryGenerators/SqlQueryGenerator.cs | 3 --- .../Features/Search/SqlServerSearchService.cs | 9 ++++----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 777269260e..f943303167 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -42,7 +42,6 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor _includeFromCteIds; private int _curFromCteIndex = -1; - private readonly string _searchParameterHash; private int _tableExpressionCounter = -1; private SqlRootExpression _rootExpression; private readonly SchemaInformation _schemaInfo; @@ -60,7 +59,6 @@ public SqlQueryGenerator( HashingSqlQueryParameterManager parameters, ISqlServerFhirModel model, SchemaInformation schemaInfo, - string searchParameterHash, SqlException sqlException = null) { EnsureArg.IsNotNull(sb, nameof(sb)); @@ -72,7 +70,6 @@ public SqlQueryGenerator( Parameters = parameters; Model = model; _schemaInfo = schemaInfo; - _searchParameterHash = searchParameterHash; if (sqlException?.Number == SqlErrorCodes.QueryProcessorNoQueryPlan) { diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs index bc069c92d6..434443ef7a 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs @@ -124,7 +124,7 @@ public override async Task SearchAsync(SearchOptions searchOptions { SqlSearchOptions sqlSearchOptions = new SqlSearchOptions(searchOptions); - SearchResult searchResult = await SearchImpl(sqlSearchOptions, null, cancellationToken); + SearchResult searchResult = await SearchImpl(sqlSearchOptions, cancellationToken); int resultCount = searchResult.Results.Count(); if (!sqlSearchOptions.IsSortWithFilter && @@ -159,7 +159,7 @@ public override async Task SearchAsync(SearchOptions searchOptions sqlSearchOptions.SortQuerySecondPhase = true; sqlSearchOptions.MaxItemCount -= resultCount; - searchResult = await SearchImpl(sqlSearchOptions, null, cancellationToken); + searchResult = await SearchImpl(sqlSearchOptions, cancellationToken); finalResultsInOrder.AddRange(searchResult.Results); searchResult = new SearchResult( @@ -188,7 +188,7 @@ public override async Task SearchAsync(SearchOptions searchOptions sqlSearchOptions.CountOnly = true; // And perform a second read. - var countOnlySearchResult = await SearchImpl(sqlSearchOptions, null, cancellationToken); + var countOnlySearchResult = await SearchImpl(sqlSearchOptions, cancellationToken); searchResult.TotalCount = countOnlySearchResult.TotalCount; } @@ -203,7 +203,7 @@ public override async Task SearchAsync(SearchOptions searchOptions return searchResult; } - private async Task SearchImpl(SqlSearchOptions sqlSearchOptions, string currentSearchParameterHash, CancellationToken cancellationToken) + private async Task SearchImpl(SqlSearchOptions sqlSearchOptions, CancellationToken cancellationToken) { Expression searchExpression = sqlSearchOptions.Expression; @@ -335,7 +335,6 @@ await _sqlRetryService.ExecuteSql( new HashingSqlQueryParameterManager(new SqlQueryParameterManager(sqlCommand.Parameters)), _model, _schemaInformation, - currentSearchParameterHash, sqlException); expression.AcceptVisitor(queryGenerator, clonedSearchOptions); From 82d772bc34be5313150e98c27807420a733e090a Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Sun, 28 Jul 2024 09:57:05 -0700 Subject: [PATCH 05/10] Database setting to reuse query plans --- .../QueryGenerators/SqlQueryGenerator.cs | 5 +- .../Features/Search/SqlServerSearchService.cs | 10 +++ .../Features/Storage/ProcessingFlag.cs | 70 +++++++++++++++++++ .../Storage/SqlServerFhirDataStore.cs | 66 ++--------------- 4 files changed, 90 insertions(+), 61 deletions(-) create mode 100644 src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index f943303167..ee48e3179a 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -53,12 +53,14 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor _reuseQueryPlans; public SqlServerSearchService( ISearchOptionsFactory searchOptionsFactory, @@ -116,6 +117,14 @@ public SqlServerSearchService( _schemaInformation = schemaInformation; _requestContextAccessor = requestContextAccessor; _compressedRawResourceConverter = compressedRawResourceConverter; + + if (_reuseQueryPlans == null) + { + lock (_locker) + { + _reuseQueryPlans ??= new ProcessingFlag("Search.ResuseQueryPlans.IsEnabled", false, _logger); + } + } } internal ISqlServerFhirModel Model => _model; @@ -335,6 +344,7 @@ await _sqlRetryService.ExecuteSql( new HashingSqlQueryParameterManager(new SqlQueryParameterManager(sqlCommand.Parameters)), _model, _schemaInformation, + _reuseQueryPlans.IsEnabled(_sqlRetryService), sqlException); expression.AcceptVisitor(queryGenerator, clonedSearchOptions); diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs new file mode 100644 index 0000000000..48c4ca0975 --- /dev/null +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs @@ -0,0 +1,70 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Data.SqlClient; +using Microsoft.Extensions.Logging; + +namespace Microsoft.Health.Fhir.SqlServer.Features.Storage +{ + internal class ProcessingFlag + { + private readonly ILogger _logger; + private bool _isEnabled; + private DateTime? _lastUpdated; + private readonly object _databaseAccessLocker = new object(); + private readonly string _parameterId; + private readonly bool _defaultValue; + + public ProcessingFlag(string parameterId, bool defaultValue, ILogger logger) + { + _parameterId = parameterId; + _defaultValue = defaultValue; + _logger = logger; + } + + public bool IsEnabled(ISqlRetryService sqlRetryService) + { + if (_lastUpdated.HasValue && (DateTime.UtcNow - _lastUpdated.Value).TotalSeconds < 600) + { + return _isEnabled; + } + + lock (_databaseAccessLocker) + { + if (_lastUpdated.HasValue && (DateTime.UtcNow - _lastUpdated.Value).TotalSeconds < 600) + { + return _isEnabled; + } + + _isEnabled = IsEnabledInDatabase(sqlRetryService); + _lastUpdated = DateTime.UtcNow; + } + + return _isEnabled; + } + + private bool IsEnabledInDatabase(ISqlRetryService sqlRetryService) + { + try + { + using var cmd = new SqlCommand(); + cmd.CommandText = "IF object_id('dbo.Parameters') IS NOT NULL SELECT Number FROM dbo.Parameters WHERE Id = @Id"; // call can be made before store is initialized + cmd.Parameters.AddWithValue("@Id", _parameterId); + var value = cmd.ExecuteScalarAsync(sqlRetryService, _logger, CancellationToken.None, disableRetries: true).Result; + return value == null ? _defaultValue : (double)value == 1; + } + catch (Exception) + { + return _defaultValue; + } + } + } +} diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs index 3ecda9fb95..72b116da30 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs @@ -59,9 +59,9 @@ internal class SqlServerFhirDataStore : IFhirDataStore, IProvideCapability private readonly SchemaInformation _schemaInformation; private readonly IModelInfoProvider _modelInfoProvider; private readonly IImportErrorSerializer _importErrorSerializer; - private static ProcessingFlag _ignoreInputLastUpdated; - private static ProcessingFlag _ignoreInputVersion; - private static ProcessingFlag _rawResourceDeduping; + private static ProcessingFlag _ignoreInputLastUpdated; + private static ProcessingFlag _ignoreInputVersion; + private static ProcessingFlag _rawResourceDeduping; private static readonly object _flagLocker = new object(); public SqlServerFhirDataStore( @@ -98,7 +98,7 @@ public SqlServerFhirDataStore( { lock (_flagLocker) { - _ignoreInputLastUpdated ??= new ProcessingFlag("MergeResources.IgnoreInputLastUpdated.IsEnabled", false, _logger); + _ignoreInputLastUpdated ??= new ProcessingFlag("MergeResources.IgnoreInputLastUpdated.IsEnabled", false, _logger); } } @@ -106,7 +106,7 @@ public SqlServerFhirDataStore( { lock (_flagLocker) { - _ignoreInputVersion ??= new ProcessingFlag("MergeResources.IgnoreInputVersion.IsEnabled", false, _logger); + _ignoreInputVersion ??= new ProcessingFlag("MergeResources.IgnoreInputVersion.IsEnabled", false, _logger); } } @@ -114,7 +114,7 @@ public SqlServerFhirDataStore( { lock (_flagLocker) { - _rawResourceDeduping ??= new ProcessingFlag("MergeResources.RawResourceDeduping.IsEnabled", true, _logger); + _rawResourceDeduping ??= new ProcessingFlag("MergeResources.RawResourceDeduping.IsEnabled", true, _logger); } } } @@ -959,59 +959,5 @@ public async Task UpdateSearchParameterIndicesAsync(ResourceWra { return await Task.FromResult((int?)null); } - - private class ProcessingFlag - { - private readonly ILogger _logger; - private bool _isEnabled; - private DateTime? _lastUpdated; - private readonly object _databaseAccessLocker = new object(); - private readonly string _parameterId; - private readonly bool _defaultValue; - - public ProcessingFlag(string parameterId, bool defaultValue, ILogger logger) - { - _parameterId = parameterId; - _defaultValue = defaultValue; - _logger = logger; - } - - public bool IsEnabled(ISqlRetryService sqlRetryService) - { - if (_lastUpdated.HasValue && (DateTime.UtcNow - _lastUpdated.Value).TotalSeconds < 600) - { - return _isEnabled; - } - - lock (_databaseAccessLocker) - { - if (_lastUpdated.HasValue && (DateTime.UtcNow - _lastUpdated.Value).TotalSeconds < 600) - { - return _isEnabled; - } - - _isEnabled = IsEnabledInDatabase(sqlRetryService); - _lastUpdated = DateTime.UtcNow; - } - - return _isEnabled; - } - - private bool IsEnabledInDatabase(ISqlRetryService sqlRetryService) - { - try - { - using var cmd = new SqlCommand(); - cmd.CommandText = "IF object_id('dbo.Parameters') IS NOT NULL SELECT Number FROM dbo.Parameters WHERE Id = @Id"; // call can be made before store is initialized - cmd.Parameters.AddWithValue("@Id", _parameterId); - var value = cmd.ExecuteScalarAsync(sqlRetryService, _logger, CancellationToken.None, disableRetries: true).Result; - return value == null ? _defaultValue : (double)value == 1; - } - catch (Exception) - { - return _defaultValue; - } - } - } } } From 9e1a264cb5ca2db00c89189ab7d071b9d9edc2af Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Sun, 28 Jul 2024 10:04:07 -0700 Subject: [PATCH 06/10] fix tests --- .../Features/Search/SqlQueryGeneratorTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs index 8bc04f7445..c1d9055fe0 100644 --- a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs +++ b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs @@ -50,8 +50,7 @@ public SqlQueryGeneratorTests() _strBuilder, parameters, _fhirModel, - _schemaInformation, - "hash"); + _schemaInformation); } [Fact] From 9557c86210d0286e7daba0ac2ab9511f26dd8fcc Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Sun, 28 Jul 2024 10:05:59 -0700 Subject: [PATCH 07/10] fix tests --- .../Features/Search/SqlQueryGeneratorTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs index c1d9055fe0..3ce3c5c89e 100644 --- a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs +++ b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs @@ -50,7 +50,8 @@ public SqlQueryGeneratorTests() _strBuilder, parameters, _fhirModel, - _schemaInformation); + _schemaInformation, + false); } [Fact] From cda5d502394eb0b0cd107f8a9c00d8706afbc771 Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Sun, 28 Jul 2024 16:58:46 -0700 Subject: [PATCH 08/10] Fixed typo --- .../Visitors/QueryGenerators/SqlQueryGenerator.cs | 8 ++++---- .../Features/Search/SqlServerSearchService.cs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index ee48e3179a..37975c5620 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -53,14 +53,14 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor("Search.ResuseQueryPlans.IsEnabled", false, _logger); + _reuseQueryPlans ??= new ProcessingFlag("Search.ReuseQueryPlans.IsEnabled", false, _logger); } } } From da3e902288571d136f47b72775e150c620c06a5b Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Mon, 29 Jul 2024 15:39:17 -0700 Subject: [PATCH 09/10] reverting exists --- .../Visitors/QueryGenerators/SqlQueryGenerator.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 37975c5620..70c2baad9d 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -1229,11 +1229,7 @@ private bool UseAppendWithJoin() } else { - // Firstly, we were still using JOINs for filtering conditions for queries with include clauses. - // This does not make sense because filtering SQL should not be dependent on existence of include. - // It also appeared that SQL Server compiles plans for JOINs 5x faster than for EXISTSs. - // Though EXISTS plans should be more "precise" I am disabling them in favor of fast compilation times. - return true; + return false; } } From a66c3332f1bcc4338509ee0845968b3dbe81a9d0 Mon Sep 17 00:00:00 2001 From: Sergey Galuzo Date: Mon, 29 Jul 2024 20:32:23 -0700 Subject: [PATCH 10/10] Tests --- .../Features/Search/SqlServerSearchService.cs | 8 +- .../Features/Storage/ProcessingFlag.cs | 7 ++ ...th.Fhir.Shared.Tests.Integration.projitems | 2 +- ...mQueryTests.cs => SqlComplexQueryTests.cs} | 101 +++++++++++++++++- 4 files changed, 113 insertions(+), 5 deletions(-) rename test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/{SqlCustomQueryTests.cs => SqlComplexQueryTests.cs} (52%) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs index 47ef373514..1d68c7ddb6 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs @@ -72,6 +72,7 @@ internal class SqlServerSearchService : SearchService private static ResourceSearchParamStats _resourceSearchParamStats; private static object _locker = new object(); private static ProcessingFlag _reuseQueryPlans; + internal const string ReuseQueryPlansParameterId = "Search.ReuseQueryPlans.IsEnabled"; public SqlServerSearchService( ISearchOptionsFactory searchOptionsFactory, @@ -122,13 +123,18 @@ public SqlServerSearchService( { lock (_locker) { - _reuseQueryPlans ??= new ProcessingFlag("Search.ReuseQueryPlans.IsEnabled", false, _logger); + _reuseQueryPlans ??= new ProcessingFlag(ReuseQueryPlansParameterId, false, _logger); } } } internal ISqlServerFhirModel Model => _model; + internal static void ResetReuseQueryPlans() + { + _reuseQueryPlans.Reset(); + } + public override async Task SearchAsync(SearchOptions searchOptions, CancellationToken cancellationToken) { SqlSearchOptions sqlSearchOptions = new SqlSearchOptions(searchOptions); diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs index 48c4ca0975..0bafb89f2a 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs @@ -30,6 +30,13 @@ public ProcessingFlag(string parameterId, bool defaultValue, ILogger lo _logger = logger; } + public string ParameterId => _parameterId; + + public void Reset() + { + _lastUpdated = null; + } + public bool IsEnabled(ISqlRetryService sqlRetryService) { if (_lastUpdated.HasValue && (DateTime.UtcNow - _lastUpdated.Value).TotalSeconds < 600) diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Microsoft.Health.Fhir.Shared.Tests.Integration.projitems b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Microsoft.Health.Fhir.Shared.Tests.Integration.projitems index f4452a8c9f..6dae7f7621 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Microsoft.Health.Fhir.Shared.Tests.Integration.projitems +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Microsoft.Health.Fhir.Shared.Tests.Integration.projitems @@ -24,7 +24,7 @@ - + diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlComplexQueryTests.cs similarity index 52% rename from test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs rename to test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlComplexQueryTests.cs index 703c998770..228ae08f51 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlComplexQueryTests.cs @@ -4,6 +4,7 @@ // ------------------------------------------------------------------------------------------------- using System; +using System.Collections.Generic; using System.Diagnostics; using System.Threading; using System.Threading.Tasks; @@ -17,6 +18,7 @@ using Microsoft.Health.Fhir.Tests.Common.FixtureParameters; using Microsoft.Health.Test.Utilities; using Microsoft.SqlServer.Management.Sdk.Sfc; +using NSubstitute.Core; using Xunit; using Xunit.Abstractions; using Xunit.Sdk; @@ -32,19 +34,112 @@ namespace Microsoft.Health.Fhir.Tests.Integration.Persistence [FhirStorageTestsFixtureArgumentSets(DataStore.SqlServer)] [Trait(Traits.OwningTeam, OwningTeam.Fhir)] [Trait(Traits.Category, Categories.Search)] - public class SqlCustomQueryTests : IClassFixture + public class SqlComplexQueryTests : IClassFixture { private readonly FhirStorageTestsFixture _fixture; private readonly ITestOutputHelper _output; - public SqlCustomQueryTests(FhirStorageTestsFixture fixture, ITestOutputHelper output) + public SqlComplexQueryTests(FhirStorageTestsFixture fixture, ITestOutputHelper output) { _fixture = fixture; _output = output; } + [Fact] + public async Task GivenSearchQuery_IfReuseQueryPlansIsEnabled_ThenPlansAreReusedAcrossDifferentParameterValues() + { + SqlServerSearchService.ResetReuseQueryPlans(); + await DisableResuseQueryPlans(); + await EnableResuseQueryPlans(); + await ResetQueryStore(); + await _fixture.SearchService.SearchAsync(KnownResourceTypes.Patient, [Tuple.Create("address-city", "City1")], CancellationToken.None); + await _fixture.SearchService.SearchAsync(KnownResourceTypes.Patient, [Tuple.Create("address-city", "City2")], CancellationToken.None); + //// values are different but plans are reused + await CheckQueryStore(2, 1); + + SqlServerSearchService.ResetReuseQueryPlans(); + await DisableResuseQueryPlans(); + await ResetQueryStore(); + await _fixture.SearchService.SearchAsync(KnownResourceTypes.Patient, [Tuple.Create("address-city", "City1")], CancellationToken.None); + await _fixture.SearchService.SearchAsync(KnownResourceTypes.Patient, [Tuple.Create("address-city", "City2")], CancellationToken.None); + //// values are different and plans are NOT reused + await CheckQueryStore(2, 2); + + SqlServerSearchService.ResetReuseQueryPlans(); + await DisableResuseQueryPlans(); + await ResetQueryStore(); + await _fixture.SearchService.SearchAsync(KnownResourceTypes.Patient, [Tuple.Create("address-city", "City1")], CancellationToken.None); + await _fixture.SearchService.SearchAsync(KnownResourceTypes.Patient, [Tuple.Create("address-city", "City1")], CancellationToken.None); + //// values are same and plans are reused + await CheckQueryStore(2, 1); + } + + private async Task CheckQueryStore(int expected_executions, int expected_compiles) + { + using var conn = await _fixture.SqlHelper.GetSqlConnectionAsync(); + using var cmd = new SqlCommand( + @" +DECLARE @count_executions int + ,@count_compiles int + ,@msg varchar(1000) +BEGIN TRY + SELECT @count_executions = sum(count_executions), @count_compiles = sum(q.count_compiles) + FROM sys.query_store_runtime_stats s + JOIN sys.query_store_plan p on p.plan_id = s.plan_id + JOIN sys.query_store_query q on q.query_id = p.query_id + JOIN sys.query_store_query_text qt on qt.query_text_id = q.query_text_id + WHERE query_sql_text LIKE '%StringSearchParam%' AND query_sql_text NOT LIKE '%sys.query_store_query%' + IF @expected_executions <> @count_executions + BEGIN + SET @msg = '@expected_executions='+convert(varchar,@expected_executions)+' <> @count_executions='+convert(varchar,@count_executions) + RAISERROR(@msg,18,127) + END + IF @expected_compiles <> @count_compiles + BEGIN + SET @msg = '@expected_compiles='+convert(varchar,@expected_compiles)+' <> @count_compiles='+convert(varchar,@count_compiles) + RAISERROR(@msg,18,127) + END +END TRY +BEGIN CATCH + THROW +END CATCH + ", + conn); + cmd.Parameters.AddWithValue("@expected_executions", expected_executions); + cmd.Parameters.AddWithValue("@expected_compiles", expected_compiles); + conn.Open(); + cmd.ExecuteNonQuery(); + } + + private async Task DisableResuseQueryPlans() + { + using var conn = await _fixture.SqlHelper.GetSqlConnectionAsync(); + using var cmd = new SqlCommand("DELETE FROM dbo.Parameters WHERE Id = @Id", conn); + cmd.Parameters.AddWithValue("@Id", SqlServerSearchService.ReuseQueryPlansParameterId); + conn.Open(); + cmd.ExecuteNonQuery(); + } + + private async Task EnableResuseQueryPlans() + { + using var conn = await _fixture.SqlHelper.GetSqlConnectionAsync(); + using var cmd = new SqlCommand("INSERT INTO dbo.Parameters (Id,Number) SELECT @Id, 1", conn); + cmd.Parameters.AddWithValue("@Id", SqlServerSearchService.ReuseQueryPlansParameterId); + conn.Open(); + cmd.ExecuteNonQuery(); + } + + private async Task ResetQueryStore() + { + using var conn = await _fixture.SqlHelper.GetSqlConnectionAsync(); + conn.Open(); + using var cmd = new SqlCommand("DECLARE @db varchar(100) = db_name() EXECUTE('ALTER DATABASE ['+@db+'] SET QUERY_STORE CLEAR')", conn); + cmd.ExecuteNonQuery(); + using var cmd2 = new SqlCommand("DECLARE @db varchar(100) = db_name() EXECUTE('ALTER DATABASE ['+@db+'] SET QUERY_STORE = ON (QUERY_CAPTURE_MODE = ALL)')", conn); + cmd2.ExecuteNonQuery(); + } + [SkippableFact] - [FhirStorageTestsFixtureArgumentSets(DataStore.SqlServer)] public async Task GivenASqlQuery_IfAStoredProcExistsWithMatchingHash_ThenStoredProcUsed() { using var conn = await _fixture.SqlHelper.GetSqlConnectionAsync();