Skip to content
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

[release/9.0] Take into account store-generated values when ordering update commands #34626

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

AndriySvyryd
Copy link
Member

Fixes #33023

Description

EF orders update commands in a way that avoids breaking store constraints, like unique indexes.
E.g. if there's a unique index on a Name column and there's currently a row with Jared and a row with Sam. The user wants to update the first row to Sam and the second row to Artak. EF needs perform the second update first.

This gets complicated when the index is over a column with a store-computed value since EF cannot predict the value it will have after an update. A workaround is to duplicate the store logic in the C# implementation. This client-side value would never be sent to the store but could be used to determine the update order. However, this workaround was broken because we also needed to avoid dependency cycles in the updates that would prevent establishing an order when a row with the same value is deleted and inserted. The way we handled this was by collapsing the delete and insert into an update and ignoring the store-generated values for ordering purposes.

This change to the ordering logic uses more fine-grained approach for determining whether a store-generated value could affect the ordering that works for both scenarios.

Customer impact

For models with a unique index over a computed column the update order can no longer be correctly determined and will throw an exception if it doesn't happen to be correct. The workaround is to send updates to the affected table one-by-one, but this would have a negative perf impact.

How found

Customer reported

Regression

Yes, from 6.0.x

Testing

Tests added.

Risk

Medium. While this code has good test coverage, due to its nature it's hard to predict all the ramifications of this change.

@AndriySvyryd AndriySvyryd requested review from artl93 and a team September 6, 2024 19:41
@SamMonoRT
Copy link
Member

@AndriySvyryd - this is for main correct? Given risk assessment (Medium), I would suggest against 9.0 release without proper testing and high confidence.

@AndriySvyryd
Copy link
Member Author

@AndriySvyryd - this is for main correct? Given risk assessment (Medium), I would suggest against 9.0 release without proper testing and high confidence.

It's intended for 9.0 if approved. I do think we have proper testing and I have high confidence it won't cause a regression. It just carries inherent higher risk due to code complexity, similar to most changes in the query pipeline.

@AndriySvyryd AndriySvyryd changed the base branch from main to release/9.0 September 6, 2024 21:18
@AndriySvyryd AndriySvyryd changed the title Take into account store-generated values when ordering update commands [release/9.0] Take into account store-generated values when ordering update commands Sep 6, 2024
@AndriySvyryd AndriySvyryd merged commit f4bfa10 into release/9.0 Sep 9, 2024
7 checks passed
@AndriySvyryd AndriySvyryd deleted the Issue33023 branch September 9, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update command order with computed columns and unique constraints
4 participants