Skip to content

DecompressedSize added to Resource table #5274

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

Jump to bottom
Open
rajithaalurims wants to merge 13 commits into
base: main
Choose a base branch
from
Open

DecompressedSize added to Resource table #5274

rajithaalurims wants to merge 13 commits into from

Conversation

@rajithaalurims
Copy link
Contributor

@rajithaalurims rajithaalurims commented Dec 11, 2025

Description

A new nullable column DecompressedSize added to Resource table.
A new table type is created to support backward and forward compatibility.
Stored procedures are changed to use data from either old or new data type depending on schema version supported on the instance.

Related issues

Addresses [issue #156583].

Testing

Describe how this change was tested.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@rajithaalurims rajithaalurims added this to the CY25Q3/2Wk12 milestone Dec 11, 2025
@rajithaalurims rajithaalurims requested a review from a team as a code owner December 11, 2025 19:40
@rajithaalurims rajithaalurims added Enhancement Enhancement on existing functionality. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Schema Version backward compatible labels Dec 11, 2025
@github-actions github-actions bot added the SQL Scripts If SQL scripts are added to the PR label Dec 11, 2025
@SergeyGaluzo
Copy link
Contributor

SergeyGaluzo commented Dec 15, 2025

@rajithaalurims I think this PR is a prerequisite to address issue https://microsofthealth.visualstudio.com/Health/_workitems/edit/156583

SergeyGaluzo
SergeyGaluzo reviewed Dec 18, 2025
View reviewed changes
src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Types/ResourceList_Temp.sql
,IsRawResourceMetaSet bit NOT NULL
,RequestMethod varchar(10) NULL
,SearchParamHash varchar(64) NULL
,DecompressedLength INT NULL
Copy link
Contributor

@SergeyGaluzo SergeyGaluzo Dec 18, 2025

Choose a reason for hiding this comment

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

Lower case

SergeyGaluzo
SergeyGaluzo previously approved these changes Dec 18, 2025
View reviewed changes
rbans96
rbans96 reviewed Dec 18, 2025
View reviewed changes
docs/arch/adr-fhir-ingested-data-size-design.md
Parameter table entries:
- FHIR_TotalDataSize: Stores ( total ingested data size + total index size) in GB
- FHIR_TotalIndexSize: Stores total index size in GB
- Both entries include timestamp of last calculation
Copy link
Contributor

@rbans96 rbans96 Dec 18, 2025

Choose a reason for hiding this comment

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

nit: Can we rename the entries to FHIR_TotalDataSizeInGB and FHIR_TotalIndexSizeInGB

Copy link
Contributor Author

@rajithaalurims rajithaalurims Dec 18, 2025

Choose a reason for hiding this comment

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

Done

docs/arch/adr-fhir-ingested-data-size-design.md
### Consequences
- Background job adds periodic database load every 4 hours
- Failure in job does not impact core FHIR server functionality
- Falure in job results in stale data size metrics until next successful run
Copy link
Contributor

@rbans96 rbans96 Dec 18, 2025

Choose a reason for hiding this comment

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

I assume we would emit failure logs from the background job. Would we need to setup any alerting/monitoring if the background job fails?
Do you think if its useful to add a section for alerts/monitors if needed at all for this design spec.

Copy link
Contributor Author

@rajithaalurims rajithaalurims Dec 18, 2025

Choose a reason for hiding this comment

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

Great question. I have added alerting and monitoring in the ADR I put in place on WP side. If there are no metrics being emitted as expected we do get ICMs.

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Tables/Resource.sql
TransactionId bigint NULL,
HistoryTransactionId bigint NULL
HistoryTransactionId bigint NULL,
DecompressedLength int NULL
Copy link
Contributor

@rbans96 rbans96 Dec 18, 2025

Choose a reason for hiding this comment

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

I understand this is nullable for backward compatibility. Do we have a test coverage to ensure we don't pass the null value/0 value for these changes, if it makes sense?

Copy link
Contributor Author

@rajithaalurims rajithaalurims Dec 18, 2025

Choose a reason for hiding this comment

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

For iteration1 null and 0 are acceptable. After iteration 2, null values are not acceptable. Once all three iterations are done, we should not have any null values in that table. I can add test coverage in iteration 2 or 3 to make sure there are no null values.

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 24, 2025
View reviewed changes
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs

var request = new PatchResourceRequest(new ResourceKey("Patient", saveResult.RawResourceElement.Id), new FhirPathPatchPayload(patchParam), bundleResourceContext: null);
var response = await Mediator.PatchResourceAsync(patchResourceRequest: request, CancellationToken.None);

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This assignment to [response](1) is useless, since its value is never read.

Copilot Autofix

AI 13 days ago

To fix this, remove the useless local variable assignment while preserving the call and its side effects. In C#, this means awaiting the PatchResourceAsync call directly without assigning its result to response.

Concretely, in test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs, in the test method WhenUpdatingAResourceWithPatchRequest_ThenDecompressedLengthShouldNotBeNull, update line 1175 so that it no longer declares or assigns response. Replace:

var response = await Mediator.PatchResourceAsync(patchResourceRequest: request, CancellationToken.None);

with:

await Mediator.PatchResourceAsync(patchResourceRequest: request, CancellationToken.None);

No additional imports, methods, or definitions are needed. This preserves the behavior (the patch still executes and is awaited) while removing the unused variable and satisfying the static analysis rule.

Suggested changeset 1
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs
--- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs
@@ -1172,7 +1172,7 @@
             var patchParam = new Parameters().AddAddPatchParameter("Patient", "name", newName);
 
             var request = new PatchResourceRequest(new ResourceKey("Patient", saveResult.RawResourceElement.Id), new FhirPathPatchPayload(patchParam), bundleResourceContext: null);
-            var response = await Mediator.PatchResourceAsync(patchResourceRequest: request, CancellationToken.None);
+            await Mediator.PatchResourceAsync(patchResourceRequest: request, CancellationToken.None);
 
             wrapper = await _fixture.DataStore.GetAsync(new ResourceKey("Patient", saveResult.RawResourceElement.Id), CancellationToken.None);
 
EOF
@@ -1172,7 +1172,7 @@
var patchParam = new Parameters().AddAddPatchParameter("Patient", "name", newName);

var request = new PatchResourceRequest(new ResourceKey("Patient", saveResult.RawResourceElement.Id), new FhirPathPatchPayload(patchParam), bundleResourceContext: null);
var response = await Mediator.PatchResourceAsync(patchResourceRequest: request, CancellationToken.None);
await Mediator.PatchResourceAsync(patchResourceRequest: request, CancellationToken.None);

wrapper = await _fixture.DataStore.GetAsync(new ResourceKey("Patient", saveResult.RawResourceElement.Id), CancellationToken.None);

Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
SergeyGaluzo
SergeyGaluzo reviewed Dec 25, 2025
View reviewed changes
src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Sprocs/GetResources.sql
,RawResource
,IsRawResourceMetaSet
,SearchParamHash
,DecompressedLength
Copy link
Contributor

@SergeyGaluzo SergeyGaluzo Dec 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@rajithaalurims Why do you need to read decompressed length in the existing stored procedures?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rbans96 rbans96 rbans96 left review comments

@SergeyGaluzo SergeyGaluzo SergeyGaluzo left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement Enhancement on existing functionality. Schema Version backward compatible SQL Scripts If SQL scripts are added to the PR

Projects

None yet

Milestone

CY25Q3/2Wk12

Development

Successfully merging this pull request may close these issues.

4 participants

@rajithaalurims @SergeyGaluzo @rbans96