From b07b02b75e3ecc4b46087aa7cfffaa68d4b789b9 Mon Sep 17 00:00:00 2001 From: SergeyGaluzo <95932081+SergeyGaluzo@users.noreply.github.com> Date: Fri, 5 Jan 2024 20:03:08 -0800 Subject: [PATCH] Removing search param and resource type ids from sql parameters (#3656) * Removing search param and resource type ids from sql parameters * Comment * OPTION (RECOMPILE) * Add hashed parameter list * comments --- .../SearchParameterQueryGenerator.cs | 26 ++++++-------- .../QueryGenerators/SqlQueryGenerator.cs | 1 + .../Search/HashingSqlQueryParameterManager.cs | 34 +++++++++++++++---- .../Features/Search/SqlServerSearchService.cs | 1 + .../Persistence/SqlCustomQueryTests.cs | 12 +++---- 5 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SearchParameterQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SearchParameterQueryGenerator.cs index ec3c1b201e..446e3e7544 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SearchParameterQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SearchParameterQueryGenerator.cs @@ -39,7 +39,7 @@ public override SearchParameterQueryGeneratorContext VisitSearchParameter(Search context.StringBuilder .Append(searchParamIdColumn, context.TableAlias) .Append(" = ") - .AppendLine(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName) + .Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true)).AppendLine() .Append("AND "); return expression.Expression.AcceptVisitor(this, context); @@ -53,7 +53,7 @@ public override SearchParameterQueryGeneratorContext VisitSortParameter(SortExpr context.StringBuilder .Append(searchParamIdColumn, context.TableAlias) .Append(" = ") - .Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName) + .Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true)) .Append(" "); return context; @@ -77,7 +77,7 @@ public override SearchParameterQueryGeneratorContext VisitMissingSearchParameter context.StringBuilder .Append(searchParamIdColumn, context.TableAlias) .Append(" = ") - .Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName) + .Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true)) .Append(" "); return context; @@ -187,7 +187,7 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleBinary(BinaryOp throw new ArgumentOutOfRangeException(binaryOperator.ToString()); } - context.StringBuilder.Append(context.Parameters.AddParameter(column, value, includeInParameterHash).ParameterName); + context.StringBuilder.Append(context.Parameters.AddParameter(column, value, includeInParameterHash)); return context; } @@ -204,17 +204,14 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleString(StringEx { case StringOperator.Contains: needsEscaping = TryEscapeValueForLike(ref value); - SqlParameter containsParameter = context.Parameters.AddParameter(column, $"%{value}%", true); - context.StringBuilder.Append(" LIKE ").Append(containsParameter.ParameterName); + context.StringBuilder.Append(" LIKE ").Append(context.Parameters.AddParameter(column, $"%{value}%", true)); break; case StringOperator.EndsWith: needsEscaping = TryEscapeValueForLike(ref value); - SqlParameter endWithParameter = context.Parameters.AddParameter(column, $"%{value}", true); - context.StringBuilder.Append(" LIKE ").Append(endWithParameter.ParameterName); + context.StringBuilder.Append(" LIKE ").Append(context.Parameters.AddParameter(column, $"%{value}", true)); break; case StringOperator.Equals: - SqlParameter equalsParameter = context.Parameters.AddParameter(column, value, true); - context.StringBuilder.Append(" = ").Append(equalsParameter.ParameterName); + context.StringBuilder.Append(" = ").Append(context.Parameters.AddParameter(column, value, true)); break; case StringOperator.NotContains: context.StringBuilder.Append(" NOT "); @@ -227,13 +224,11 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleString(StringEx goto case StringOperator.StartsWith; case StringOperator.StartsWith: needsEscaping = TryEscapeValueForLike(ref value); - SqlParameter startsWithParameter = context.Parameters.AddParameter(column, $"{value}%", true); - context.StringBuilder.Append(" LIKE ").Append(startsWithParameter.ParameterName); + context.StringBuilder.Append(" LIKE ").Append(context.Parameters.AddParameter(column, $"{value}%", true)); break; case StringOperator.LeftSideStartsWith: needsEscaping = TryEscapeValueForLike(ref value); - SqlParameter leftParameter = context.Parameters.AddParameter(column, $"{value}", true); - context.StringBuilder.Append(leftParameter.ParameterName).Append(" LIKE "); + context.StringBuilder.Append(context.Parameters.AddParameter(column, $"{value}", true)).Append(" LIKE "); AppendColumnName(context, column, expression); context.StringBuilder.Append("+'%'"); break; @@ -259,8 +254,7 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleString(StringEx context.StringBuilder.Append(" AND "); AppendColumnName(context, column, expression); - SqlParameter equalsParameter = context.Parameters.AddParameter(column, value, true); - context.StringBuilder.Append(" = ").Append(equalsParameter.ParameterName); + context.StringBuilder.Append(" = ").Append(context.Parameters.AddParameter(column, value, true)); } context.StringBuilder.Append(" COLLATE ").Append(expression.IgnoreCase ? DefaultCaseInsensitiveCollation : DefaultCaseSensitiveCollation); 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 f2b0e2bb5d..b612fea2e3 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 @@ -337,6 +337,7 @@ private void AddHash() StringBuilder.Append("/* HASH "); Parameters.AppendHash(StringBuilder); + Parameters.AppendHashedParameterNames(StringBuilder); StringBuilder.AppendLine(" */"); } } diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/HashingSqlQueryParameterManager.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/HashingSqlQueryParameterManager.cs index b09a8ce80d..46fbce6cc1 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/HashingSqlQueryParameterManager.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/HashingSqlQueryParameterManager.cs @@ -12,6 +12,7 @@ using System.Security.Cryptography; using EnsureThat; using Microsoft.Data.SqlClient; +using Microsoft.Health.Fhir.SqlServer.Features.Schema.Model; using Microsoft.Health.SqlServer; using Microsoft.Health.SqlServer.Features.Schema.Model; using Microsoft.Health.SqlServer.Features.Storage; @@ -35,7 +36,7 @@ public HashingSqlQueryParameterManager(SqlQueryParameterManager inner) public bool HasParametersToHash => _setToHash.Count > 0; /// - /// Add a parameter to the SQL command. + /// Add a parameter to the SQL command if it is not ResourceTypeId and not SearchParamId. /// /// The CLR column type /// The table column the parameter is bound to. @@ -44,14 +45,14 @@ public HashingSqlQueryParameterManager(SqlQueryParameterManager inner) /// Whether this parameter should be included in the hash of the overall parameters. /// If true, this parameter will prevent other identical queries with a different value for this parameter from re-using the query plan. /// - /// The SQL parameter. - public SqlParameter AddParameter(Column column, T value, bool includeInHash) + /// SQL parameter or input value depending on whether input was added to the list of parameters. + public object AddParameter(Column column, T value, bool includeInHash) { return AddParameter((Column)column, value, includeInHash); } /// - /// Add a parameter to the SQL command. + /// Add a parameter to the SQL command if it is not ResourceTypeId and not SearchParamId. /// /// The table column the parameter is bound to. /// The parameter value @@ -59,9 +60,16 @@ public SqlParameter AddParameter(Column column, T value, bool includeInHas /// Whether this parameter should be included in the hash of the overall parameters. /// If true, this parameter will prevent other identical queries with a different value for this parameter from re-using the query plan. /// - /// The SQL parameter. - public SqlParameter AddParameter(Column column, object value, bool includeInHash) + /// SQL parameter or input value depending on whether input was added to the list of parameters. + public object AddParameter(Column column, object value, bool includeInHash) { + if (column.Metadata.Name == VLatest.Resource.ResourceTypeId.Metadata.Name // logic uses "ResourceTypeId" string value. Resource table is chosen arbitrarily. + || column.Metadata.Name == VLatest.ReferenceSearchParam.ReferenceResourceTypeId.Metadata.Name + || column.Metadata.Name == VLatest.TokenSearchParam.SearchParamId.Metadata.Name) // logic uses "SearchParamId" string value. We don't have cross table column sharing concept yet, so to avoid hardcoding TokenSearchParam is arbitrarily chosen. + { + return value; + } + SqlParameter parameter = _inner.AddParameter(column, value); if (includeInHash) { @@ -182,6 +190,20 @@ public void AppendHash(IndentedStringBuilder stringBuilder) stringBuilder.Append(hashChars[..hashCharsLength]); } + /// + /// Appends comma delimited list of names of hashed parameters + /// + /// A string builder to append the list to. + public void AppendHashedParameterNames(IndentedStringBuilder stringBuilder) + { + var first = true; + foreach (var param in _setToHash) + { + stringBuilder.Append($"{(first ? " params=" : ",")}{param}"); + first = false; + } + } + private static void WriteAndAdvance(Span buffer, ref int currentIndex, ref IncrementalHash incrementalHash, T element) where T : struct { diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs index 1d164fc31a..9535777703 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs @@ -882,6 +882,7 @@ private void LogSqlCommand(SqlCommand sqlCommand) sb.AppendLine(); } + sb.AppendLine("OPTION (RECOMPILE)"); // enables query compilation with provided parameter values in debugging sb.AppendLine($"-- execution timeout = {sqlCommandWrapper.CommandTimeout} sec."); _sqlRetryService.TryLogEvent("Search", "Start", sb.ToString(), null, CancellationToken.None); _logger.LogInformation("{SqlQuery}", sb.ToString()); diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs index b87b0cb135..de22234693 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs @@ -129,15 +129,11 @@ private void AddSproc(string hash) { _fixture.SqlHelper.ExecuteSqlCmd(@$" CREATE OR ALTER PROCEDURE [dbo].[CustomQuery_{hash}] - @p0 smallint + @p0 datetime2 ,@p1 datetime2 - ,@p2 smallint - ,@p3 datetime2 - ,@p4 smallint - ,@p5 nvarchar(256) - ,@p6 smallint - ,@p7 nvarchar(256) - ,@p8 int + ,@p2 nvarchar(256) + ,@p3 nvarchar(256) + ,@p4 int AS set nocount on SELECT DISTINCT r.ResourceTypeId, r.ResourceId, r.Version, r.IsDeleted, r.ResourceSurrogateId, r.RequestMethod, CAST(1 AS bit) AS IsMatch, CAST(0 AS bit) AS IsPartial, r.IsRawResourceMetaSet, r.SearchParamHash, r.RawResource