Database schema optimizations #4731
Open
+12,709
−568
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* History separation V0 * Forgotten resource * replace current by view * TRUNCATE * HardDelete * delete history v0 * missed 64 and removed dead code from SQL query generator * Removed history from current in merge * Triggers * tests and tran * disable/enable indexes * rollback hard delete and merge * rollback delete invisible history * TRUNCATE -> DELETE * line * leftovers * PK check * Cosmetic * WHERE nanes * Get resources with forced indexes * Removed redundant where * Adding feature flag for raw resource dedupping * Enable invisible history by default * parameters * commit * fixed typo * 65 + more realistic diff * adjusted update trigger * right trigger * next iteration * special chars * Added delete and update * \r * Adding script runner * Added verification * cosmetic * HOLDLOCK hint * removed incorrect comments * RT * lock timeout * 180 and remove empty * no changes in update resource search params * blob rewriter tool * Revert "no changes in update resource search params" This reverts commit b2e0c38. * comment * Generic disable indexes and update search params * Dummy resources * fixes * Dummy records based on surr id * adjust test to filter dummy rows * exclude history and current * Adding PerfTest V-1 * Get asyn wrapper without type string to id function * tool * not exists on data copy * packages back * Added comments. * Correct filtered index on resource current * Added redundant IsHistory=0 to index * deduping * Removed dummy records * pp-p * history clause * deduping * testing parameters * get resource by type and surr id range with many versions * skip large databases * Added calls to fhir * added conflict * max retries = 3 * database pings * examples * reverse * start closer * put logic * Create * Move 65 to 84 * Fixes after merge * Adding RawResources table * change capture tweeks * Test fix * Added MI * inline insert * Repeat on updates
… users/sergal/newschemarefsep
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerSchemaUpgradeTests.cs
Fixed
Show fixed
Hide fixed
Comment on lines
+1059
to
+1063
| catch (Exception e) | ||
| { | ||
| Console.WriteLine($"uri={uri} error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetDDMSIncludeIterate", "Error", $"id={patientId} error={e.Message}", null, CancellationToken.None).Wait(); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 9 days ago
To fix the problem, we need to replace the generic catch (Exception e) clause with specific exception types that are likely to be thrown by the code within the try block. In this case, since the code is making HTTP requests and processing the response, we should catch exceptions such as HttpRequestException, TaskCanceledException, and possibly others related to network and I/O operations.
- Identify the specific exceptions that can be thrown by
_httpClient.GetAsync(uri).Resultandresponse.Content.ReadAsStringAsync().Result. - Replace the generic
catch (Exception e)with multiplecatchblocks for these specific exceptions. - Ensure that the error handling logic remains the same, i.e., logging the error and continuing execution.
Suggested changeset
1
tools/PerfTester/Program.cs
| @@ -1035,6 +1035,16 @@ | ||
| } | ||
| catch (HttpRequestException e) | ||
| { | ||
| Console.WriteLine($"uri={uri} HTTP request error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetObservationsForPatient", "Error", $"id={patientId} HTTP request error={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| catch (TaskCanceledException e) | ||
| { | ||
| Console.WriteLine($"uri={uri} request timed out={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetObservationsForPatient", "Error", $"id={patientId} request timed out={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Console.WriteLine($"uri={uri} error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetObservationsForPatient", "Error", $"id={patientId} error={e.Message}", null, CancellationToken.None).Wait(); | ||
| Console.WriteLine($"uri={uri} unexpected error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetObservationsForPatient", "Error", $"id={patientId} unexpected error={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| @@ -1058,6 +1068,16 @@ | ||
| } | ||
| catch (HttpRequestException e) | ||
| { | ||
| Console.WriteLine($"uri={uri} HTTP request error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetDDMSIncludeIterate", "Error", $"id={patientId} HTTP request error={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| catch (TaskCanceledException e) | ||
| { | ||
| Console.WriteLine($"uri={uri} request timed out={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetDDMSIncludeIterate", "Error", $"id={patientId} request timed out={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Console.WriteLine($"uri={uri} error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetDDMSIncludeIterate", "Error", $"id={patientId} error={e.Message}", null, CancellationToken.None).Wait(); | ||
| Console.WriteLine($"uri={uri} unexpected error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetDDMSIncludeIterate", "Error", $"id={patientId} unexpected error={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| @@ -1081,6 +1101,16 @@ | ||
| } | ||
| catch (HttpRequestException e) | ||
| { | ||
| Console.WriteLine($"uri={uri} HTTP request error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetPatientRevIncludeObservations", "Error", $"id={patientId} HTTP request error={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| catch (TaskCanceledException e) | ||
| { | ||
| Console.WriteLine($"uri={uri} request timed out={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetPatientRevIncludeObservations", "Error", $"id={patientId} request timed out={e.Message}", null, CancellationToken.None).Wait(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Console.WriteLine($"uri={uri} error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetPatientRevIncludeObservations", "Error", $"id={patientId} error={e.Message}", null, CancellationToken.None).Wait(); | ||
| Console.WriteLine($"uri={uri} unexpected error={e.Message}"); | ||
| _store.TryLogEvent("Diag.GetPatientRevIncludeObservations", "Error", $"id={patientId} unexpected error={e.Message}", null, CancellationToken.None).Wait(); | ||
| } |
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Optimizations:
Deployment is dependent on small schema change creating CurrentResource view. #4755
https://microsofthealth.visualstudio.com/Health/_workitems/edit/135369
Question:
Can this PR be cut into 2 smaller ones?
Answer:
No, if we want to avoid re-writing Resource table twice. Here is why. There are 2 main schema optimizations: #1 moving historical resources into separate table, which allows to reduce number of indexes, and, hence, save space, and #2 resource id integer mapping. Though these optimizations are independent, both require changes in the way resources are stored. #1 requires adding not only history table but also extra table to store raw resources (this reduces update latency as old raw resource stays in the same place). #2 requires new ResourceIdIntMap table, and also changes main resource row, where ResourceId string is replaced by its integer mapped value ResourceIdInt.
Question:
This PR has ~13K lines changed. Why is it that big?
Answer:
FHIR solution is not using database projects:
#1 Currently even a single line change in a single stored procedure results in a full schema SQL script added to PR. This alone is 6.5K code lines.
#2 Modified, even slightly, stored procedures have to be added in full to the diff.sql (~2K lines).
Other reasons:
#1 Data migration requires intermediate stored procedures and views to access old and new schema. This almost doubles the total changed lines in diff.sql.
#2 FHIR uses generator for database wrapper classes. This generator does not work with views, so, to bypass this limitation, SQL scripts have "artificial" table statements to generate wrappers, which (tables) are immediately dropped, and "replaced" by view statements (same names, so table wrapper classes can be reused for views). This significantly contributes to changed lines.
#3 There are ~300 lines of code specific to read/write of raw resources from/to ADLS. This code is dormant until ADLS is enabled.
#4 There are ~936 lines of test code changes.