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.

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

Add database setting to reuse query plans #3991

Merged
merged 11 commits into from
Jul 30, 2024
Merged

Add database setting to reuse query plans #3991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8fe0c3c
Disabling EXISTS
SergeyGaluzo Jul 27, 2024
eea073a
Comment
SergeyGaluzo Jul 27, 2024
4cd13e5
spelling
SergeyGaluzo Jul 27, 2024
63c0a4d
Removed not used parameter currentSearchParametersHash
SergeyGaluzo Jul 28, 2024
82d772b
Database setting to reuse query plans
SergeyGaluzo Jul 28, 2024
9e1a264
fix tests
SergeyGaluzo Jul 28, 2024
49190c2
Merge branch 'users/sergal/reducecompilationtime' into users/sergal/r…
SergeyGaluzo Jul 28, 2024
9557c86
fix tests
SergeyGaluzo Jul 28, 2024
cda5d50
Fixed typo
SergeyGaluzo Jul 28, 2024
da3e902
reverting exists
SergeyGaluzo Jul 29, 2024
a66c333
Tests
SergeyGaluzo Jul 30, 2024
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
2 changes: 1 addition & 1 deletion src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public SqlQueryGeneratorTests()
parameters,
_fhirModel,
_schemaInformation,
"hash");
false);
}

[Fact]
Expand Down
28 changes: 14 additions & 14 deletions ....Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor<SearchOptions, ob
private List<string> _includeFromCteIds;

private int _curFromCteIndex = -1;
private readonly string _searchParameterHash;
private int _tableExpressionCounter = -1;
private SqlRootExpression _rootExpression;
private readonly SchemaInformation _schemaInfo;
Expand All @@ -54,13 +53,14 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor<SearchOptions, ob
private int _searchParamCount = 0;
private bool previousSqlQueryGeneratorFailure = false;
private int maxTableExpressionCountLimitForExists = 5;
private bool _reuseQueryPlans;
Dismissed Show dismissed Hide dismissed

public SqlQueryGenerator(
IndentedStringBuilder sb,
HashingSqlQueryParameterManager parameters,
ISqlServerFhirModel model,
SchemaInformation schemaInfo,
string searchParameterHash,
bool reuseQueryPlans,
SqlException sqlException = null)
{
EnsureArg.IsNotNull(sb, nameof(sb));
Expand All @@ -72,7 +72,7 @@ public SqlQueryGenerator(
Parameters = parameters;
Model = model;
_schemaInfo = schemaInfo;
_searchParameterHash = searchParameterHash;
_reuseQueryPlans = reuseQueryPlans;

if (sqlException?.Number == SqlErrorCodes.QueryProcessorNoQueryPlan)
{
Expand Down Expand Up @@ -320,7 +320,7 @@ private void AddOptionClause()

private void AddHash()
{
if (Parameters.HasParametersToHash) // hash cannot be last comment as it will not be stored in query store
if (Parameters.HasParametersToHash && !_reuseQueryPlans) // hash cannot be last comment as it will not be stored in query store
{
// Add a hash of (most of the) parameter values as a comment.
// We do this to avoid re-using query plans unless two queries have
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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)
{
Expand Down
25 changes: 20 additions & 5 deletions src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@
private readonly ISqlQueryHashCalculator _queryHashCalculator;
private static ResourceSearchParamStats _resourceSearchParamStats;
private static object _locker = new object();
private static ProcessingFlag<SqlServerSearchService> _reuseQueryPlans;
Fixed Show fixed Hide fixed
Dismissed Show dismissed Hide dismissed
internal const string ReuseQueryPlansParameterId = "Search.ReuseQueryPlans.IsEnabled";

public SqlServerSearchService(
ISearchOptionsFactory searchOptionsFactory,
Expand Down Expand Up @@ -116,15 +118,28 @@
_schemaInformation = schemaInformation;
_requestContextAccessor = requestContextAccessor;
_compressedRawResourceConverter = compressedRawResourceConverter;

if (_reuseQueryPlans == null)
{
lock (_locker)
{
_reuseQueryPlans ??= new ProcessingFlag<SqlServerSearchService>(ReuseQueryPlansParameterId, false, _logger);
Dismissed Show dismissed Hide dismissed
}
}
}

internal ISqlServerFhirModel Model => _model;

internal static void ResetReuseQueryPlans()
{
_reuseQueryPlans.Reset();
}

public override async Task<SearchResult> SearchAsync(SearchOptions searchOptions, CancellationToken cancellationToken)
{
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 &&
Expand Down Expand Up @@ -159,7 +174,7 @@
sqlSearchOptions.SortQuerySecondPhase = true;
sqlSearchOptions.MaxItemCount -= resultCount;

searchResult = await SearchImpl(sqlSearchOptions, null, cancellationToken);
searchResult = await SearchImpl(sqlSearchOptions, cancellationToken);

finalResultsInOrder.AddRange(searchResult.Results);
searchResult = new SearchResult(
Expand Down Expand Up @@ -188,7 +203,7 @@
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;
}
Expand All @@ -203,7 +218,7 @@
return searchResult;
}

private async Task<SearchResult> SearchImpl(SqlSearchOptions sqlSearchOptions, string currentSearchParameterHash, CancellationToken cancellationToken)
private async Task<SearchResult> SearchImpl(SqlSearchOptions sqlSearchOptions, CancellationToken cancellationToken)
{
Expression searchExpression = sqlSearchOptions.Expression;

Expand Down Expand Up @@ -335,7 +350,7 @@
new HashingSqlQueryParameterManager(new SqlQueryParameterManager(sqlCommand.Parameters)),
_model,
_schemaInformation,
currentSearchParameterHash,
_reuseQueryPlans.IsEnabled(_sqlRetryService),
sqlException);

expression.AcceptVisitor(queryGenerator, clonedSearchOptions);
Expand Down
77 changes: 77 additions & 0 deletions src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ProcessingFlag.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// -------------------------------------------------------------------------------------------------
// 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<TLogger>
Copy link
Member

Choose a reason for hiding this comment

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

Why is ProcessingFlag class using another class's logger?

Copy link
Contributor Author

@SergeyGaluzo SergeyGaluzo Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just kept current behavior when moved ProcessingFlag from private to internal. Does it hurt?
BTW That is what we do in other places. See CustomQueries, for example, which takes search service logger.

{
private readonly ILogger<TLogger> _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<TLogger> logger)
{
_parameterId = parameterId;
_defaultValue = defaultValue;
_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)
{
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;
Dismissed Show dismissed Hide dismissed
}
catch (Exception)
{
return _defaultValue;
}
Dismissed Show dismissed Hide dismissed
}
}
}
66 changes: 6 additions & 60 deletions src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<SqlServerFhirDataStore> _ignoreInputLastUpdated;
Dismissed Show dismissed Hide dismissed
private static ProcessingFlag<SqlServerFhirDataStore> _ignoreInputVersion;
Dismissed Show dismissed Hide dismissed
private static ProcessingFlag<SqlServerFhirDataStore> _rawResourceDeduping;
Dismissed Show dismissed Hide dismissed
private static readonly object _flagLocker = new object();

public SqlServerFhirDataStore(
Expand Down Expand Up @@ -98,23 +98,23 @@ public SqlServerFhirDataStore(
{
lock (_flagLocker)
{
_ignoreInputLastUpdated ??= new ProcessingFlag("MergeResources.IgnoreInputLastUpdated.IsEnabled", false, _logger);
_ignoreInputLastUpdated ??= new ProcessingFlag<SqlServerFhirDataStore>("MergeResources.IgnoreInputLastUpdated.IsEnabled", false, _logger);
Dismissed Show dismissed Hide dismissed
}
}

if (_ignoreInputVersion == null)
{
lock (_flagLocker)
{
_ignoreInputVersion ??= new ProcessingFlag("MergeResources.IgnoreInputVersion.IsEnabled", false, _logger);
_ignoreInputVersion ??= new ProcessingFlag<SqlServerFhirDataStore>("MergeResources.IgnoreInputVersion.IsEnabled", false, _logger);
Dismissed Show dismissed Hide dismissed
}
}

if (_rawResourceDeduping == null)
{
lock (_flagLocker)
{
_rawResourceDeduping ??= new ProcessingFlag("MergeResources.RawResourceDeduping.IsEnabled", true, _logger);
_rawResourceDeduping ??= new ProcessingFlag<SqlServerFhirDataStore>("MergeResources.RawResourceDeduping.IsEnabled", true, _logger);
Dismissed Show dismissed Hide dismissed
}
}
}
Expand Down Expand Up @@ -959,59 +959,5 @@ public async Task<ResourceWrapper> UpdateSearchParameterIndicesAsync(ResourceWra
{
return await Task.FromResult((int?)null);
}

private class ProcessingFlag
{
private readonly ILogger<SqlServerFhirDataStore> _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<SqlServerFhirDataStore> 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;
}
}
}
}
}
2 changes: 1 addition & 1 deletion ...th.Fhir.Shared.Tests.Integration/Microsoft.Health.Fhir.Shared.Tests.Integration.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<Compile Include="$(MSBuildThisFileDirectory)Features\Operations\Reindex\ReindexSearchTests.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Features\Smart\SmartSearchTests.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Persistence\SqlAzurePipelinesWorkloadIdentityAuthenticationProvider.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Persistence\SqlCustomQueryTests.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Persistence\SqlComplexQueryTests.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Persistence\FhirStorageVersioningPolicyTests.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Persistence\SearchParameterStatusDataStoreTests.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Persistence\SqlRetryServiceTests.cs" />
Expand Down
Loading
Loading