Add SQL performance option #4785
Conversation
LTA-Thinking
added
Enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (inputResource is not Parameters) | ||
| { | ||
| throw new RequestNotValidException(string.Format(Resources.UnsupportedResourceType, inputResource.GetType().ToString())); | ||
| throw new RequestNotValidException(string.Format(Api.Resources.UnsupportedResourceType, inputResource.GetType().ToString())); |
Check notice
Code scanning / CodeQL
Redundant ToString() call Note
| if (string.Equals(useQueryCache, "true", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return await SearchImpl(sqlSearchOptions, true, cancellationToken); | ||
| } | ||
| else if (string.Equals(useQueryCache, "false", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return await SearchImpl(sqlSearchOptions, false, cancellationToken); | ||
| } | ||
| else if (string.Equals(useQueryCache, "both", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| _logger.LogInformation("Running search with and without query cache."); | ||
| var stopwatch = Stopwatch.StartNew(); | ||
|
|
||
| using var tokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| var token = tokenSource.Token; | ||
|
|
||
| var tryWithQueryCache = SearchImpl(sqlSearchOptions, true, token); | ||
| var tryWithoutQueryCache = SearchImpl(sqlSearchOptions, false, token); | ||
|
|
||
| var result = await Task.WhenAny(tryWithQueryCache, tryWithoutQueryCache); | ||
| await tokenSource.CancelAsync(); | ||
|
|
||
| _logger.LogInformation("First search completed in {ElapsedMilliseconds}ms, query cache enabled: {QueryCacheEnabled}.", stopwatch.ElapsedMilliseconds, result == tryWithQueryCache); | ||
| return await result; | ||
| } |
There was a problem hiding this comment.
Do we want to expose this on such a granular level?
There was a problem hiding this comment.
I didn't see a way to let the user control this setting with less visibility.
I know we try and keep the database operations opaque to the user, but since we've seen this setting can dramatically affect query performance I think it would be a good lever to be able to pull if needed.
| { | ||
| return await SearchImpl(sqlSearchOptions, false, cancellationToken); | ||
| } | ||
| else if (string.Equals(useQueryCache, "both", StringComparison.OrdinalIgnoreCase)) |
|
I think the description is now out of date for "x-ms-query-latency-over-efficiency" |
| { | ||
| internal class QueryCacheSetting | ||
| { | ||
| public const string Enabled = "enabled"; |
There was a problem hiding this comment.
C# doesn't support string enumerations. Since this is mapping from a user inputted header value this is the best I could think of.
There was a problem hiding this comment.
It's true you would need something like: Enum.TryParse(x, out QueryCacheSettingsEnum cacheSetting);
But more of a suggestion.
| && useQueryCacheObj != null) | ||
| { | ||
| var useQueryCache = Convert.ToString(useQueryCacheObj); | ||
| if (string.Equals(useQueryCache, QueryCacheSetting.Enabled, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
nit: perhaps reading the value once and convert to enum instead of multiple string comparisions
| @@ -357,7 +402,7 @@ await _sqlRetryService.ExecuteSql( | |||
| new HashingSqlQueryParameterManager(new SqlQueryParameterManager(sqlCommand.Parameters)), | |||
| _model, | |||
| _schemaInformation, | |||
| _reuseQueryPlans.IsEnabled(_sqlRetryService), | |||
| reuseQueryPlans, | |||
There was a problem hiding this comment.
This allows the setting to use or not use query plans to be controlled from an input parameter, which is then controlled by the header value parsing code.
Description
Adds support for the x-ms-query-cache header for Gen2. This gives the user the option to control if SQL uses query caching on the request level. This can enable, disable, or have SQL run both and use the better performing one. This helps in situations where a user has some queries that perform better with query caching and some that perform better without it.
Related issues
Addresses User Story 137387: DDMS: SQL search with and without cache
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch