-
Notifications
You must be signed in to change notification settings - Fork 567
Schema change to add DecompressedLength column to Resource table #5247
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.
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
base: main
Are you sure you want to change the base?
Conversation
| ,@TokenQuantityCompositeSearchParams dbo.TokenQuantityCompositeSearchParamList READONLY | ||
| ,@TokenStringCompositeSearchParams dbo.TokenStringCompositeSearchParamList READONLY | ||
| ,@TokenNumberNumberCompositeSearchParams dbo.TokenNumberNumberCompositeSearchParamList READONLY | ||
| ,@DecompressedOverridesJson NVARCHAR(MAX) = NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing JSON on each merge call sounds expensive. Instead, we should add decompressed length to ResourceList table type. There should be multiple iterations to get compatibility and final clean state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was debating between changing ResourceList table type but as you mentioned that needs multiple iterations of code release to get final clean slate.
From what I have researched additional cost for parsing takes around few hundred milli seconds for rows around 10000.
I will discuss offline with you about which option to go with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. But imagine that we start adding extra JSON vars for each new column we add... This looks like antipattern. Also, there are extra joins to @Resources, and they are extra cost too... All of these are just not applicable to the case of table type change. BTW You should get fully functioning code on first iteration...
| TransactionId bigint NULL, | ||
| HistoryTransactionId bigint NULL | ||
| HistoryTransactionId bigint NULL, | ||
| DecompressedLength decimal(36,18) NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it cannot be int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have used INT. I will change
| BEGIN TRY | ||
| INSERT INTO dbo.Parameters (Id, Char) SELECT 'Adding DecompressedLength', 'LogEvent' | ||
| EXECUTE dbo.LogEvent @Process='Adding DecompressedLength',@Status='Start' | ||
| BEGIN TRANSACTION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why transaction is needed. Please explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to drop and recreate procedure in the same transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to drop/create, but use alter. If you break up the transaction, what negative effects do you expect?
Description
A new Nullable column DecompressedLength added to Resource table.
This will be used to store size of raw resource before compressing it.
This change is both backward and forward compatible.
Checking schema version before passing additional parameter to MergeResource SP to avoid "Procedure or function 'MergeResources' has too many arguments specified." error.
Related issues
Addresses [issue #156583].
Testing
Tested with old code new schema and new code old schema
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)